diff --git a/ChangeLog b/ChangeLog index 12ee964495..b83203c6d7 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,8 @@ +2353. [func] razvan + Restricted location of configured scripts in loaded hook + libraries. + (Gitlab #3849) + 2352. [bug] razvan Fix error handling when detecting a global reservation for the client and global reservatons are explicitly disabled in the diff --git a/doc/sphinx/arm/hooks-legal-log.rst b/doc/sphinx/arm/hooks-legal-log.rst index 149d1c0acc..a36e3beb37 100644 --- a/doc/sphinx/arm/hooks-legal-log.rst +++ b/doc/sphinx/arm/hooks-legal-log.rst @@ -165,6 +165,10 @@ script and are configured with the following settings: - ``postrotate`` - an external executable or script called with the name of the file that was opened. Kea does not wait for the process to finish. +These executables must be stored in the ``"[kea-install-dir]/share/kea/scripts/"`` +directory which can be overridden at startup by setting the environment variable +``KEA_HOOK_SCRIPTS_PATH`` to a different path. + Custom formatting can be enabled for logging information that can be extracted either from the client's request packet or from the server's response packet. Use with caution as this might affect server performance. diff --git a/doc/sphinx/arm/hooks-run-script.rst b/doc/sphinx/arm/hooks-run-script.rst index b8912b7974..31f8ad1574 100644 --- a/doc/sphinx/arm/hooks-run-script.rst +++ b/doc/sphinx/arm/hooks-run-script.rst @@ -38,6 +38,10 @@ If the ``sync`` parameter is ``false``, then the script will launch and Kea will not wait for the execution to finish, causing all the OUT parameters of the script (including the next step) to be ignored. +The external script must be stored in the ``"[kea-install-dir]/share/kea/scripts/"`` +directory which can be overridden at startup by setting the environment variable +``KEA_HOOK_SCRIPTS_PATH`` to a different path. + .. note:: The script inherits all privileges from the server which calls it. diff --git a/doc/sphinx/arm/security.rst b/doc/sphinx/arm/security.rst index aa4a055a50..8817da7107 100644 --- a/doc/sphinx/arm/security.rst +++ b/doc/sphinx/arm/security.rst @@ -426,6 +426,8 @@ the following table: +-------------------------------------+---------------------------------------+-------------------------------+ | Unix Sockets | ``var/run/kea`` | ``KEA_CONTROL_SOCKET_DIR`` | +-------------------------------------+---------------------------------------+-------------------------------+ +| Scripts used by hook libraries | ``share/kea/scripts/`` | ``KEA_HOOK_SCRIPTS_PATH`` | ++-------------------------------------+---------------------------------------+-------------------------------+ .. note: diff --git a/src/hooks/dhcp/forensic_log/rotating_file.cc b/src/hooks/dhcp/forensic_log/rotating_file.cc index 010bc91fa9..4f9b553fe0 100644 --- a/src/hooks/dhcp/forensic_log/rotating_file.cc +++ b/src/hooks/dhcp/forensic_log/rotating_file.cc @@ -7,6 +7,7 @@ #include #include +#include #include #include #include @@ -25,6 +26,7 @@ using namespace isc::util; using namespace isc::dhcp; using namespace isc::data; using namespace isc::db; +using namespace isc::hooks; using namespace std; namespace isc { @@ -104,6 +106,7 @@ RotatingFile::apply(const DatabaseConnection::ParameterMap& parameters) { if (!prerotate_.empty()) { try { + HookLibraryScriptsChecker::validatePath(prerotate_); ProcessSpawn process(ProcessSpawn::ASYNC, prerotate_); } catch (const isc::Exception& ex) { isc_throw(LegalLogMgrError, "Invalid 'prerotate' parameter: " << ex.what()); @@ -112,6 +115,7 @@ RotatingFile::apply(const DatabaseConnection::ParameterMap& parameters) { if (!postrotate_.empty()) { try { + HookLibraryScriptsChecker::validatePath(postrotate_); ProcessSpawn process(ProcessSpawn::ASYNC, postrotate_); } catch (const isc::Exception& ex) { isc_throw(LegalLogMgrError, "Invalid 'postrotate' parameter: " << ex.what()); 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 798a0ece97..1434819019 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 @@ -14,6 +14,7 @@ #include #include #include +#include #include #include #include @@ -26,6 +27,7 @@ using namespace isc; using namespace isc::data; using namespace isc::db; using namespace isc::dhcp; +using namespace isc::hooks; using namespace isc::legal_log; using namespace isc::test; using namespace std; @@ -41,6 +43,7 @@ DbLogger legal_log_db_logger(legal_log_logger, legal_log_db_message_map); struct LegalLogMgrTest : ::testing::Test { /// @brief Constructor. LegalLogMgrTest() : legal_log_dir_env_var_("KEA_LEGAL_LOG_DIR") { + HookLibraryScriptsChecker::getHookScriptsPath(true, TEST_DATA_BUILDDIR); } /// @brief Destructor. diff --git a/src/hooks/dhcp/forensic_log/tests/test_utils.h b/src/hooks/dhcp/forensic_log/tests/test_utils.h index 8b1335334b..8e380bd43e 100644 --- a/src/hooks/dhcp/forensic_log/tests/test_utils.h +++ b/src/hooks/dhcp/forensic_log/tests/test_utils.h @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -147,6 +148,7 @@ public: /// @brief Constructor RotatingFileTest() : time_(RotatingFileTest::getTime()) { + HookLibraryScriptsChecker::getHookScriptsPath(true, TEST_DATA_BUILDDIR); } /// @brief Destructor diff --git a/src/hooks/dhcp/run_script/libloadtests/load_unload_unittests.cc b/src/hooks/dhcp/run_script/libloadtests/load_unload_unittests.cc index 3679d40e01..d68d176a14 100644 --- a/src/hooks/dhcp/run_script/libloadtests/load_unload_unittests.cc +++ b/src/hooks/dhcp/run_script/libloadtests/load_unload_unittests.cc @@ -13,6 +13,7 @@ #include #include +#include #include #include @@ -32,6 +33,7 @@ class RunScriptLibLoadTest : public isc::test::LibLoadTest { public: /// @brief Constructor RunScriptLibLoadTest() : LibLoadTest(LIBRUN_SCRIPT_SO) { + HookLibraryScriptsChecker::getHookScriptsPath(true, TEST_DATA_BUILDDIR); } /// @brief Destructor diff --git a/src/hooks/dhcp/run_script/libloadtests/meson.build b/src/hooks/dhcp/run_script/libloadtests/meson.build index 59b53c350c..1cc805d19b 100644 --- a/src/hooks/dhcp/run_script/libloadtests/meson.build +++ b/src/hooks/dhcp/run_script/libloadtests/meson.build @@ -7,6 +7,7 @@ dhcp_run_script_libload_tests = executable( 'load_unload_unittests.cc', 'run_unittests.cc', cpp_args: [ + f'-DTEST_DATA_BUILDDIR="@current_build_dir@"', f'-DLIBRUN_SCRIPT_SO="@TOP_BUILD_DIR@/src/hooks/dhcp/run_script/libdhcp_run_script.so"', f'-DRUN_SCRIPT_TEST_SH="@TOP_BUILD_DIR@/src/hooks/dhcp/run_script/tests/run_script_test.sh"', ], diff --git a/src/hooks/dhcp/run_script/run_script.h b/src/hooks/dhcp/run_script/run_script.h index 5ab6d2ab3f..201f0c56c1 100644 --- a/src/hooks/dhcp/run_script/run_script.h +++ b/src/hooks/dhcp/run_script/run_script.h @@ -18,6 +18,7 @@ #include #include #include +#include #include namespace isc { @@ -217,6 +218,7 @@ public: /// /// @param name The name of the target script. void setName(const std::string& name) { + isc::hooks::HookLibraryScriptsChecker::validatePath(name); name_ = name; } diff --git a/src/hooks/dhcp/run_script/tests/meson.build b/src/hooks/dhcp/run_script/tests/meson.build index 55f1296d04..c69064f437 100644 --- a/src/hooks/dhcp/run_script/tests/meson.build +++ b/src/hooks/dhcp/run_script/tests/meson.build @@ -16,6 +16,7 @@ dhcp_run_script_lib_tests = executable( 'run_script_unittests.cc', 'run_unittests.cc', cpp_args: [ + f'-DTEST_DATA_BUILDDIR="@current_build_dir@"', f'-DRUN_SCRIPT_LIB_SO="@TOP_BUILD_DIR@/src/hooks/dhcp/run_script/libdhcp_run_script.so"', f'-DTEST_LOG_FILE="@current_build_dir@/test.log"', f'-DRUN_SCRIPT_TEST_SH="@current_build_dir@/run_script_test.sh"', diff --git a/src/hooks/dhcp/run_script/tests/run_script_unittests.cc b/src/hooks/dhcp/run_script/tests/run_script_unittests.cc index 66a700da62..d6acccf258 100644 --- a/src/hooks/dhcp/run_script/tests/run_script_unittests.cc +++ b/src/hooks/dhcp/run_script/tests/run_script_unittests.cc @@ -824,6 +824,7 @@ public: /// @brief Constructor. RunScriptTest() : co_manager_(new CalloutManager(1)), io_service_(new IOService()) { + HookLibraryScriptsChecker::getHookScriptsPath(true, TEST_DATA_BUILDDIR); ProcessSpawn::setIOService(io_service_); clearLogFile(); } diff --git a/src/lib/hooks/hooks_parser.cc b/src/lib/hooks/hooks_parser.cc index 26553c2cb8..26beef75ca 100644 --- a/src/lib/hooks/hooks_parser.cc +++ b/src/lib/hooks/hooks_parser.cc @@ -27,8 +27,32 @@ namespace hooks { namespace { // Singleton PathChecker to set and hold valid hooks library path. PathCheckerPtr hooks_path_checker_; + + // Singleton PathChecker to set and hold valid scripts path (scripts loaded by hook libraries). + PathCheckerPtr hook_scripts_path_checker_; }; +std::string +HookLibraryScriptsChecker::getHookScriptsPath(bool reset /* = false */, const std::string explicit_path /* = "" */) { + if (!hook_scripts_path_checker_ || reset) { + hook_scripts_path_checker_.reset(new PathChecker(DEFAULT_HOOK_SCRIPTS_PATH, "KEA_HOOK_SCRIPTS_PATH")); + if (!explicit_path.empty()) { + hook_scripts_path_checker_->getPath(true, explicit_path); + } + } + + return (hook_scripts_path_checker_->getPath()); +} + +std::string +HookLibraryScriptsChecker::validatePath(const std::string libpath) { + if (!hook_scripts_path_checker_) { + getHookScriptsPath(); + } + + return (hook_scripts_path_checker_->validatePath(libpath)); +} + std::string HooksLibrariesParser::getHooksPath(bool reset /* = false */, const std::string explicit_path /* = "" */) { if (!hooks_path_checker_ || reset) { diff --git a/src/lib/hooks/hooks_parser.h b/src/lib/hooks/hooks_parser.h index 41e79b5eea..dfe0c8b835 100644 --- a/src/lib/hooks/hooks_parser.h +++ b/src/lib/hooks/hooks_parser.h @@ -83,6 +83,33 @@ public: const std::string explicit_path = ""); }; +class HookLibraryScriptsChecker { +public: + /// @brief Validates a script path (script loaded by a hook) against the + /// supported path. + /// + /// @param libpath script path to validate. + /// + /// @return validated path + static std::string validatePath(const std::string libpath); + + /// @brief Fetches the supported script path. + /// + /// The first call to this function with no arguments will set the default + /// hooks path to either the value of DEFAULT_HOOK_SCRIPTS_PATH or the environment + /// variable KEA_HOOK_SCRIPTS_PATH if it is defined. Subsequent calls with no + /// arguments will simply return this value. + /// + /// @param reset recalculate when true, defaults to false. This is for + /// testing purposes only. + /// @param explicit_path set default hooks path to this value. This is + /// for testing purposes only. + /// + /// @return String containing the default script path. + static std::string getHookScriptsPath(bool reset = false, + const std::string explicit_path = ""); +}; + } // namespace isc::hooks } // namespace isc diff --git a/src/lib/hooks/meson.build b/src/lib/hooks/meson.build index 69f5198b62..c3d214fcde 100644 --- a/src/lib/hooks/meson.build +++ b/src/lib/hooks/meson.build @@ -13,7 +13,10 @@ kea_hooks_lib = shared_library( 'library_manager.cc', 'library_manager_collection.cc', 'server_hooks.cc', - cpp_args: [f'-DDEFAULT_HOOKS_PATH="@DEFAULT_HOOKS_PATH@"'], + cpp_args: [ + f'-DDEFAULT_HOOK_SCRIPTS_PATH="@PREFIX@/@DATADIR@/kea/scripts"', + f'-DDEFAULT_HOOKS_PATH="@DEFAULT_HOOKS_PATH@"' + ], include_directories: [include_directories('.')] + INCLUDES, install: true, install_dir: LIBDIR,