[#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 6434b356a7f893f4865ed9f643d8ed5045035656..023bd242fa66ec70db5ab397b6ee35df42a70f5e 100644 (file)
--- 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 91c4ed93f4110d804cee0344a009f13566e20725..0844511bf33881f44d9d5dc1979e3072bd20ca43 100644 (file)
--- 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 d50a7dbdc4420bc956bc5a2b2ae9bf860ce39eac..010bc91fa9fcd9fabe5b64bd90bd18025b183d25 100644 (file)
--- 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 eb61a236113d27231db02fbbbf3d1a3a34c1738d..105d1aea7947778c27d063ca26dfcf2982528ac7 100644 (file)
--- 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 <dhcpsrv/legal_log_db_log.h>
 #include <legal_log_log.h>
 #include <rotating_file.h>
+#include <testutils/env_var_wrapper.h>
 
 #include <gtest/gtest.h>
 
@@ -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<RotatingFile&>(*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<RotatingFile&>(*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 a9f951fc7f2163af4e88e375aef7bbc922626874..c8c9456082c5e15832934f427590c456002dee80 100644 (file)
--- 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