From: Thomas Markwalder Date: Wed, 14 May 2025 18:08:38 +0000 (-0400) Subject: [#3831] Added path validation, updated tests X-Git-Tag: Kea-2.7.9~37 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=36972ffcb7417cdc84daa9dad2b05b0014f68306;p=thirdparty%2Fkea.git [#3831] Added path validation, updated tests /src/hooks/dhcp/forensic_log/libloadtests/load_unload_unittests.cc Updated tests /src/hooks/dhcp/forensic_log/rotating_file.cc RotatingFile::apply{) - fetch default from singleton /src/hooks/dhcp/forensic_log/tests/legal_log_mgr_unittests.cc TEST_F(LegalLogMgrTest, pathValidation) TEST_F(LegalLogMgrTest, pathEnvVarOverride) - new tests /src/lib/dhcpsrv/legal_log_mgr.* LegalLogMgr::parseFile() - validate path Updated the ARM. --- diff --git a/doc/sphinx/arm/hooks-legal-log.rst b/doc/sphinx/arm/hooks-legal-log.rst index 6434b356a7..023bd242fa 100644 --- a/doc/sphinx/arm/hooks-legal-log.rst +++ b/doc/sphinx/arm/hooks-legal-log.rst @@ -127,7 +127,18 @@ For :iscman:`kea-dhcp6`, the configuration is: The hook library parameters for the text file configuration are: - ``path`` - the directory in which the forensic file(s) will be written. - The default value is ``[prefix]/var/lib/kea``. The directory must exist. + The default value is ``[kea-install-dir]/var/log/kea``. The directory + must exist. + +.. note:: + + As of Kea 2.7.9, log files may only be written to the output directory + determined during compilation as: ``"[kea-install-dir]/var/log/kea"``. This + path may be overridden at startup by setting the environment variable + ``KEA_LEGAL_LOG_DIR`` to the desired path. If a path other than this value + is used in ``path``, Kea will emit an error and refuse to start or if already + running, if already running, log an unrecoverable error. For ease of use + simply omit the ``path`` parameter. - ``base-name`` - an arbitrary value which is used in conjunction with the current system date to form the current forensic file name. It diff --git a/src/hooks/dhcp/forensic_log/libloadtests/load_unload_unittests.cc b/src/hooks/dhcp/forensic_log/libloadtests/load_unload_unittests.cc index 91c4ed93f4..0844511bf3 100644 --- a/src/hooks/dhcp/forensic_log/libloadtests/load_unload_unittests.cc +++ b/src/hooks/dhcp/forensic_log/libloadtests/load_unload_unittests.cc @@ -53,12 +53,14 @@ public: /// over from previous tests LegalLibLoadTest() : LibLoadTest(LEGAL_LOG_LIB_SO) { reset(); + setLogTestPath(); } /// @brief Destructor /// Removes files that may be left over from previous tests virtual ~LegalLibLoadTest() { reset(); + resetLogPath(); } /// @brief Removes files that may be left over from previous tests @@ -74,6 +76,18 @@ public: } } + /// @brief Sets the log path for log files. + /// @param custom_path path to use as the log file path. + void setLogTestPath(const std::string explicit_path = "") { + LegalLogMgr::getLogPath(true, (!explicit_path.empty() ? + explicit_path : TEST_DATA_BUILDDIR)); + } + + /// @brief Resets the log path to LEGAL_LOG_DIR. + void resetLogPath() { + LegalLogMgr::getLogPath(true); + } + /// @brief Checks if the given file exists /// /// @return true if the file exists, false if it does not @@ -115,7 +129,6 @@ TEST_F(LegalLibLoadTest, invalidType) { data::ElementPtr params = data::Element::createMap(); params->set("type", data::Element::create("no-such-type")); // Still set path and base-name in the case the library loads - params->set("path", data::Element::create(TEST_DATA_BUILDDIR)); params->set("base-name", data::Element::create("nobody-should-use-this")); // Attempting to Load the library should fail. @@ -129,10 +142,12 @@ TEST_F(LegalLibLoadTest, invalidType) { // one runs the risk of harming a legal file opened with default values // by a live instance of Kea. TEST_F(LegalLibLoadTest, validLoad) { + // Set family and daemon's proc name. + isc::dhcp::CfgMgr::instance().setFamily(AF_INET); + isc::process::Daemon::setProcName("kea-dhcp4"); // Prepare parameters for the callout parameters library. data::ElementPtr params = data::Element::createMap(); - params->set("path", data::Element::create(TEST_DATA_BUILDDIR)); params->set("base-name", data::Element::create("test-legal")); params->set("request-parser-format", data::Element::create("'request'")); params->set("response-parser-format", data::Element::create("'response'")); @@ -169,7 +184,6 @@ TEST_F(LegalLibLoadTest, invalidDaemonLoad) { // Prepare parameters for the callout parameters library. // Even if loads will fail still avoid defaults! data::ElementPtr params = data::Element::createMap(); - params->set("path", data::Element::create(TEST_DATA_BUILDDIR)); params->set("base-name", data::Element::create("test-legal")); // V4 is invalid when family is AF_INET6. @@ -203,7 +217,6 @@ TEST_F(LegalLibLoadTest, fileValidLoad) { // Prepare parameters for the callout parameters library. data::ElementPtr params = data::Element::createMap(); - params->set("path", data::Element::create(TEST_DATA_BUILDDIR)); params->set("base-name", data::Element::create("test-legal")); params->set("type", data::Element::create("logfile")); @@ -280,7 +293,6 @@ TEST_F(PgSqlLegalLibLoadTest, PgSqlValidLoad) { params->set("password", data::Element::create("keatest")); // Still set path and base-name in the case the library switches to file - params->set("path", data::Element::create(TEST_DATA_BUILDDIR)); params->set("base-name", data::Element::create("nobody-should-use-this")); // Load the library. Is there a way to check more than return code? @@ -298,7 +310,6 @@ TEST_F(PgSqlLegalLibLoadTest, PgSqlInvalidLoad) { params->set("password", data::Element::create("keatest")); // Still set path and base-name in the case the library switches to file - params->set("path", data::Element::create(TEST_DATA_BUILDDIR)); params->set("base-name", data::Element::create("nobody-should-use-this")); // Attempting to Load the library should fail. @@ -338,7 +349,6 @@ TEST_F(MySqlLegalLibLoadTest, MySqlValidLoad) { params->set("password", data::Element::create("keatest")); // Still set path and base-name in the case the library switches to file - params->set("path", data::Element::create(TEST_DATA_BUILDDIR)); params->set("base-name", data::Element::create("nobody-should-use-this")); // Load the library. Is there a way to check more than return code? @@ -356,7 +366,6 @@ TEST_F(MySqlLegalLibLoadTest, MySqlInvalidLoad) { params->set("password", data::Element::create("keatest")); // Still set path and base-name in the case the library switches to file - params->set("path", data::Element::create(TEST_DATA_BUILDDIR)); params->set("base-name", data::Element::create("nobody-should-use-this")); // Attempting to Load the library should fail. diff --git a/src/hooks/dhcp/forensic_log/rotating_file.cc b/src/hooks/dhcp/forensic_log/rotating_file.cc index d50a7dbdc4..010bc91fa9 100644 --- a/src/hooks/dhcp/forensic_log/rotating_file.cc +++ b/src/hooks/dhcp/forensic_log/rotating_file.cc @@ -37,7 +37,7 @@ RotatingFile::RotatingFile(const DatabaseConnection::ParameterMap& parameters) void RotatingFile::apply(const DatabaseConnection::ParameterMap& parameters) { - string path(LEGAL_LOG_DIR); + string path(LegalLogMgr::getLogPath()); string base("kea-legal"); RotatingFile::TimeUnit unit(RotatingFile::TimeUnit::Day); int64_t count(1); diff --git a/src/hooks/dhcp/forensic_log/tests/legal_log_mgr_unittests.cc b/src/hooks/dhcp/forensic_log/tests/legal_log_mgr_unittests.cc index eb61a23611..105d1aea79 100644 --- a/src/hooks/dhcp/forensic_log/tests/legal_log_mgr_unittests.cc +++ b/src/hooks/dhcp/forensic_log/tests/legal_log_mgr_unittests.cc @@ -16,6 +16,7 @@ #include #include #include +#include #include @@ -26,6 +27,7 @@ using namespace isc::data; using namespace isc::db; using namespace isc::dhcp; using namespace isc::legal_log; +using namespace isc::test; using namespace std; namespace { @@ -37,11 +39,16 @@ DbLogger legal_log_db_logger(legal_log_logger, legal_log_db_message_map); /// @brief Test fixture struct LegalLogMgrTest : ::testing::Test { + /// @brief Construtor. + LegalLogMgrTest() : legal_log_dir_env_var_("KEA_LEGAL_LOG_DIR") { + } + /// @brief Destructor. virtual ~LegalLogMgrTest() = default; /// @brief Called before each test. virtual void SetUp() override { + setLogTestPath(); // Clean up from past tests. LegalLogMgrFactory::delAllBackends(); } @@ -51,6 +58,7 @@ struct LegalLogMgrTest : ::testing::Test { // Clean up from past tests. LegalLogMgrFactory::delAllBackends(); reset(); + resetLogPath(); } /// @brief Removes files that may be left over from previous tests @@ -66,8 +74,23 @@ struct LegalLogMgrTest : ::testing::Test { } } + /// @brief Sets the log path for log files. + /// @param custom_path path to use as the log file path. + void setLogTestPath(const std::string explicit_path = "") { + LegalLogMgr::getLogPath(true, (!explicit_path.empty() ? + explicit_path : TEST_DATA_BUILDDIR)); + } + + /// @brief Resets the log path to LEGAL_LOG_DIR. + void resetLogPath() { + LegalLogMgr::getLogPath(true); + } + /// @brief Initializer. RotatingFileInit init_; + + /// @brief RAII wrapper for KEA_LEGAL_LOG_DIR env variable. + EnvVarWrapper legal_log_dir_env_var_; }; // Verifies output of genDurationString() @@ -135,17 +158,70 @@ TEST_F(LegalLogMgrTest, emptyVectorDump) { EXPECT_TRUE(LegalLogMgr::vectorHexDump(bytes).empty()); } +// Verify path validation +TEST_F(LegalLogMgrTest, pathValidation) { + isc::db::DatabaseConnection::ParameterMap map; + EXPECT_NO_THROW(LegalLogMgr::parseConfig(ConstElementPtr(), map)); + EXPECT_NO_THROW(LegalLogMgrFactory::addBackend(map)); + EXPECT_TRUE(LegalLogMgrFactory::instance()); + + // Default path should be OK. + ElementPtr params = Element::createMap(); + params->set("base-name", Element::create("name")); + ASSERT_NO_THROW_LOG(LegalLogMgr::parseFile(params, map)); + + // Valid path should be OK. + params = Element::createMap(); + params->set("path", Element::create(LegalLogMgr::getLogPath())); + ASSERT_NO_THROW_LOG(LegalLogMgr::parseFile(params, map)); + + // Invalid path should NOT be OK. + params = Element::createMap(); + params->set("path", Element::create("/tmp")); + std::ostringstream os; + os << "invalid path specified: '/tmp', supported path is '" + << LegalLogMgr::getLogPath() << "'"; + ASSERT_THROW_MSG(LegalLogMgr::parseFile(params, map), BadValue, os.str()); +} + +// Verify env variable path override. +TEST_F(LegalLogMgrTest, pathEnvVarOverride) { + legal_log_dir_env_var_.setValue("/tmp"); + resetLogPath(); + isc::db::DatabaseConnection::ParameterMap map; + EXPECT_NO_THROW(LegalLogMgr::parseConfig(ConstElementPtr(), map)); + EXPECT_NO_THROW(LegalLogMgrFactory::addBackend(map)); + EXPECT_TRUE(LegalLogMgrFactory::instance()); + + // Default path should be OK. + ElementPtr params = Element::createMap(); + params->set("base-name", Element::create("name")); + ASSERT_NO_THROW_LOG(LegalLogMgr::parseFile(params, map)); + + // Valid path should be OK. + params = Element::createMap(); + params->set("path", Element::create("/tmp")); + ASSERT_NO_THROW_LOG(LegalLogMgr::parseFile(params, map)); + + // Invalid path should NOT be OK. + params = Element::createMap(); + params->set("path", Element::create("/bogus")); + std::ostringstream os; + os << "invalid path specified: '/bogus', supported path is '" + << LegalLogMgr::getLogPath() << "'"; + ASSERT_THROW_MSG(LegalLogMgr::parseFile(params, map), BadValue, os.str()); +} + // Verify that parsing extra parameters for rotate file works TEST_F(LegalLogMgrTest, parseExtraRotatingFileParameters) { isc::db::DatabaseConnection::ParameterMap map; EXPECT_NO_THROW(LegalLogMgr::parseConfig(ConstElementPtr(), map)); - map["path"] = TEST_DATA_BUILDDIR; EXPECT_NO_THROW(LegalLogMgrFactory::addBackend(map)); EXPECT_TRUE(LegalLogMgrFactory::instance()); RotatingFile& rotating_file = dynamic_cast(*LegalLogMgrFactory::instance()); ElementPtr params = Element::createMap(); - params->set("path", Element::create("path")); + params->set("path", Element::create(LegalLogMgr::getLogPath())); params->set("base-name", Element::create("name")); params->set("time-unit", Element::create(0)); @@ -178,7 +254,6 @@ TEST_F(LegalLogMgrTest, parseExtraRotatingFileParameters) { // Verify that parsing extra parameters works TEST_F(LegalLogMgrTest, parseExtraParameters) { db::DatabaseConnection::ParameterMap map; - map["path"] = "path"; map["base-name"] = "name"; ASSERT_NO_THROW(LegalLogMgrFactory::instance().reset(new RotatingFile(map))); ElementPtr params = Element::createMap(); @@ -208,13 +283,12 @@ TEST_F(LegalLogMgrTest, parseExtraParameters) { TEST_F(LegalLogMgrTest, fileNoParameters) { db::DatabaseConnection::ParameterMap map; EXPECT_NO_THROW(LegalLogMgr::parseFile(ConstElementPtr(), map)); - map["path"] = TEST_DATA_BUILDDIR; ASSERT_NO_THROW(LegalLogMgrFactory::instance().reset(new RotatingFile(map))); ASSERT_NO_THROW(LegalLogMgrFactory::instance()->open()); RotatingFile& rotating_file = dynamic_cast(*LegalLogMgrFactory::instance()); map.clear(); rotating_file.apply(map); - EXPECT_EQ(rotating_file.getPath(), LEGAL_LOG_DIR); + EXPECT_EQ(rotating_file.getPath(), LegalLogMgr::getLogPath()); EXPECT_EQ(rotating_file.getBaseName(), "kea-legal"); } diff --git a/src/lib/dhcpsrv/legal_log_mgr.cc b/src/lib/dhcpsrv/legal_log_mgr.cc index a9f951fc7f..c8c9456082 100644 --- a/src/lib/dhcpsrv/legal_log_mgr.cc +++ b/src/lib/dhcpsrv/legal_log_mgr.cc @@ -172,6 +172,11 @@ LegalLogMgr::parseFile(const ConstElementPtr& parameters, DatabaseConnection::Pa for (char const* const& key : { "path", "base-name", "time-unit", "prerotate", "postrotate" }) { ConstElementPtr const value(parameters->get(key)); if (value) { + if (key == std::string("path")) { + auto valid_path = validatePath(value->stringValue()); + file_parameters.emplace(key, valid_path); + } + file_parameters.emplace(key, value->stringValue()); } } @@ -388,12 +393,12 @@ LegalLogMgr::getLogPath(bool reset /* = false */, const std::string explicit_pat std::string LegalLogMgr::validatePath(const std::string logpath, - bool enforce_path /* = true */) { + bool enforce_path /* = true */) { if (!legal_log_path_checker_) { getLogPath(); } - return (legal_log_path_checker_->validatePath(logpath, enforce_path)); + return (legal_log_path_checker_->validateDirectory(logpath, enforce_path)); } } // namespace dhcp