From a71d44c5710aa567ece753f6b487f6a3dbf375e8 Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Wed, 11 Jun 2025 14:30:15 -0400 Subject: [PATCH] [#3848] Detect authentication risks Throw or Warn if API end points do not use some form of authentication Throw or Warn if 'user', 'password' - API end points Throw or Warn if 'secret' is used - TSIG Disable/enable security for UTs as needed modified: src/bin/agent/tests/ca_cfg_mgr_unittests.cc modified: src/bin/agent/tests/ca_response_creator_unittests.cc modified: src/bin/agent/tests/get_config_unittest.cc modified: src/bin/d2/tests/d2_cfg_mgr_unittests.cc modified: src/bin/d2/tests/d2_command_unittest.cc modified: src/bin/d2/tests/d2_controller_unittests.cc modified: src/bin/d2/tests/d2_http_command_unittest.cc modified: src/bin/d2/tests/d2_process_unittests.cc modified: src/bin/d2/tests/d2_simple_parser_unittest.cc modified: src/bin/d2/tests/get_config_unittest.cc modified: src/bin/dhcp4/tests/config_parser_unittest.cc modified: src/bin/dhcp4/tests/dhcp4_srv_unittest.cc modified: src/bin/dhcp4/tests/dhcp4_test_utils.cc modified: src/bin/dhcp4/tests/get_config_unittest.cc modified: src/bin/dhcp4/tests/get_config_unittest.cc.skel modified: src/bin/dhcp4/tests/http_control_socket_unittest.cc modified: src/bin/dhcp6/tests/config_parser_unittest.cc modified: src/bin/dhcp6/tests/dhcp6_srv_unittest.cc modified: src/bin/dhcp6/tests/dhcp6_test_utils.cc modified: src/bin/dhcp6/tests/get_config_unittest.cc modified: src/bin/dhcp6/tests/get_config_unittest.cc.skel modified: src/bin/dhcp6/tests/http_control_socket_unittest.cc modified: src/lib/config/tests/http_command_config_unittests.cc modified: src/lib/d2srv/d2_config.cc modified: src/lib/d2srv/d2_messages.cc modified: src/lib/d2srv/d2_messages.h modified: src/lib/d2srv/d2_messages.mes modified: src/lib/http/auth_messages.cc modified: src/lib/http/auth_messages.h modified: src/lib/http/auth_messages.mes modified: src/lib/http/basic_auth_config.cc modified: src/lib/http/tests/basic_auth_config_unittests.cc modified: src/lib/testutils/dhcp_test_lib.sh.in --- src/bin/agent/tests/ca_cfg_mgr_unittests.cc | 2 + .../tests/ca_response_creator_unittests.cc | 7 ++ src/bin/agent/tests/get_config_unittest.cc | 3 + src/bin/d2/tests/d2_cfg_mgr_unittests.cc | 5 ++ src/bin/d2/tests/d2_command_unittest.cc | 2 + src/bin/d2/tests/d2_controller_unittests.cc | 8 ++ src/bin/d2/tests/d2_http_command_unittest.cc | 4 + src/bin/d2/tests/d2_process_unittests.cc | 4 + src/bin/d2/tests/d2_simple_parser_unittest.cc | 4 + src/bin/d2/tests/get_config_unittest.cc | 2 + src/bin/dhcp4/tests/config_parser_unittest.cc | 3 + src/bin/dhcp4/tests/dhcp4_srv_unittest.cc | 1 + src/bin/dhcp4/tests/dhcp4_test_utils.cc | 2 + src/bin/dhcp4/tests/get_config_unittest.cc | 6 +- .../dhcp4/tests/get_config_unittest.cc.skel | 8 +- .../tests/http_control_socket_unittest.cc | 3 + src/bin/dhcp6/tests/config_parser_unittest.cc | 5 ++ src/bin/dhcp6/tests/dhcp6_srv_unittest.cc | 1 + src/bin/dhcp6/tests/dhcp6_test_utils.cc | 3 + src/bin/dhcp6/tests/get_config_unittest.cc | 3 +- .../dhcp6/tests/get_config_unittest.cc.skel | 6 ++ .../tests/http_control_socket_unittest.cc | 1 + .../tests/http_command_config_unittests.cc | 1 + src/lib/d2srv/d2_config.cc | 10 +++ src/lib/d2srv/d2_messages.cc | 2 + src/lib/d2srv/d2_messages.h | 1 + src/lib/d2srv/d2_messages.mes | 14 +++ src/lib/http/auth_messages.cc | 4 + src/lib/http/auth_messages.h | 2 + src/lib/http/auth_messages.mes | 12 +++ src/lib/http/basic_auth_config.cc | 16 ++++ .../http/tests/basic_auth_config_unittests.cc | 88 ++++++++++++++++++- src/lib/testutils/dhcp_test_lib.sh.in | 8 +- 33 files changed, 231 insertions(+), 10 deletions(-) diff --git a/src/bin/agent/tests/ca_cfg_mgr_unittests.cc b/src/bin/agent/tests/ca_cfg_mgr_unittests.cc index edd8ec716b..9e99ed6d8b 100644 --- a/src/bin/agent/tests/ca_cfg_mgr_unittests.cc +++ b/src/bin/agent/tests/ca_cfg_mgr_unittests.cc @@ -436,11 +436,13 @@ public: virtual void SetUp() { resetHooksPath(); setSocketTestPath(); + file::PathChecker::enableEnforcement(false); } virtual void TearDown() { resetHooksPath(); resetSocketPath(); + file::PathChecker::enableEnforcement(true); } /// @brief Sets the Hooks path from which hooks can be loaded. diff --git a/src/bin/agent/tests/ca_response_creator_unittests.cc b/src/bin/agent/tests/ca_response_creator_unittests.cc index e45c2f363b..19cf3339c2 100644 --- a/src/bin/agent/tests/ca_response_creator_unittests.cc +++ b/src/bin/agent/tests/ca_response_creator_unittests.cc @@ -16,6 +16,7 @@ #include #include #include +#include #include #include #include @@ -29,6 +30,7 @@ using namespace isc::data; using namespace isc::hooks; using namespace isc::http; using namespace isc::process; +using namespace isc::util; namespace ph = std::placeholders; namespace { @@ -46,6 +48,7 @@ public: : DControllerTest(CtrlAgentController::instance), response_creator_(), request_(response_creator_.createNewHttpRequest()) { + file::PathChecker::enableEnforcement(true); // Deregisters commands. CtrlAgentCommandMgr::instance().deregisterAll(); CtrlAgentCommandMgr::instance(). @@ -76,6 +79,7 @@ public: CtrlAgentCommandMgr::instance().deregisterAll(); HooksManager::prepareUnloadLibraries(); static_cast(HooksManager::unloadLibraries()); + file::PathChecker::enableEnforcement(true); } /// @brief Fills request context with required data to create new request. @@ -435,6 +439,7 @@ TEST_F(CtrlAgentResponseCreatorTest, basicAuth) { // required but not provided by request using the hook. TEST_F(CtrlAgentResponseCreatorTest, hookNoAuth) { setBasicContext(request_); + file::PathChecker::enableEnforcement(false); // Body: "list-commands" is natively supported by the command manager. // We add a random value in the extra entry: see next unit test @@ -493,6 +498,7 @@ TEST_F(CtrlAgentResponseCreatorTest, hookNoAuth) { // auth response provided by the hool. TEST_F(CtrlAgentResponseCreatorTest, hookNoAuthHeaders) { setBasicContext(request_); + file::PathChecker::enableEnforcement(false); // Body: "list-commands" is natively supported by the command manager. // We add a random value in the extra entry: see next unit test @@ -548,6 +554,7 @@ TEST_F(CtrlAgentResponseCreatorTest, hookNoAuthHeaders) { // Test successful server response when the client is authenticated. TEST_F(CtrlAgentResponseCreatorTest, hookBasicAuth) { setBasicContext(request_); + file::PathChecker::enableEnforcement(false); // Body: "list-commands" is natively supported by the command manager. // We add a random value in the extra entry: diff --git a/src/bin/agent/tests/get_config_unittest.cc b/src/bin/agent/tests/get_config_unittest.cc index eac18255a1..d799d1427c 100644 --- a/src/bin/agent/tests/get_config_unittest.cc +++ b/src/bin/agent/tests/get_config_unittest.cc @@ -146,12 +146,14 @@ public: // Create fresh context. resetConfiguration(); setSocketTestPath(); + file::PathChecker::enableEnforcement(true); } ~CtrlAgentGetCfgTest() { resetConfiguration(); resetHooksPath(); resetSocketPath(); + file::PathChecker::enableEnforcement(true); } /// @brief Sets the Hooks path from which hooks can be loaded. @@ -287,6 +289,7 @@ public: /// Test a configuration TEST_F(CtrlAgentGetCfgTest, simple) { setHooksTestPath(); + file::PathChecker::enableEnforcement(false); // get the simple configuration std::string simple_file = string(CFG_EXAMPLES) + "/" + "simple.json"; diff --git a/src/bin/d2/tests/d2_cfg_mgr_unittests.cc b/src/bin/d2/tests/d2_cfg_mgr_unittests.cc index 384190dde1..051388895f 100644 --- a/src/bin/d2/tests/d2_cfg_mgr_unittests.cc +++ b/src/bin/d2/tests/d2_cfg_mgr_unittests.cc @@ -55,12 +55,14 @@ public: D2CfgMgrTest():cfg_mgr_(new D2CfgMgr()), d2_params_() { resetHooksPath(); resetSocketPath(); + file::PathChecker::enableEnforcement(true); } /// @brief Destructor ~D2CfgMgrTest() { resetHooksPath(); resetSocketPath(); + file::PathChecker::enableEnforcement(true); } /// @brief Sets the Hooks path from which hooks can be loaded. @@ -501,6 +503,7 @@ TEST(D2CfgMgr, construction) { TEST_F(D2CfgMgrTest, fullConfig) { setHooksTestPath(); setSocketTestPath(); + file::PathChecker::enableEnforcement(false); // Create a configuration with all of application level parameters, plus // both the forward and reverse ddns managers. Both managers have two @@ -958,6 +961,7 @@ TEST_F(D2CfgMgrTest, matchReverse) { TEST_F(D2CfgMgrTest, configPermutations) { std::string test_file = testDataFile("d2_cfg_tests.json"); isc::data::ConstElementPtr tests; + file::PathChecker::enableEnforcement(false); // Read contents of the file and parse it as JSON. Note it must contain // all valid JSON, we aren't testing JSON parsing. @@ -1013,6 +1017,7 @@ TEST_F(D2CfgMgrTest, configPermutations) { /// @brief Tests comments. TEST_F(D2CfgMgrTest, comments) { setSocketTestPath(); + file::PathChecker::enableEnforcement(false); std::string config = "{ " "\"comment\": \"D2 config\" , " "\"ip-address\" : \"192.168.1.33\" , " diff --git a/src/bin/d2/tests/d2_command_unittest.cc b/src/bin/d2/tests/d2_command_unittest.cc index 52360a22cb..53a997d27e 100644 --- a/src/bin/d2/tests/d2_command_unittest.cc +++ b/src/bin/d2/tests/d2_command_unittest.cc @@ -129,6 +129,7 @@ public: : server_(NakedD2Controller::instance()) { setSocketTestPath(); ::remove(socket_path_.c_str()); + file::PathChecker::enableEnforcement(false); } /// @brief Destructor. @@ -144,6 +145,7 @@ public: CommandMgr::instance().deregisterAll(); UnixCommandMgr::instance().setConnectionTimeout(TIMEOUT_DHCP_SERVER_RECEIVE_COMMAND); resetSocketPath(); + file::PathChecker::enableEnforcement(true); } /// @brief Returns pointer to the server's IO service. diff --git a/src/bin/d2/tests/d2_controller_unittests.cc b/src/bin/d2/tests/d2_controller_unittests.cc index 243e830ab4..600dc8d580 100644 --- a/src/bin/d2/tests/d2_controller_unittests.cc +++ b/src/bin/d2/tests/d2_controller_unittests.cc @@ -11,6 +11,7 @@ #include #include #include +#include #include #include @@ -22,6 +23,7 @@ using namespace isc::asiolink::test; using namespace isc::process; +using namespace isc::util; using namespace boost::posix_time; namespace isc { @@ -45,6 +47,12 @@ public: /// Note the constructor passes in the static D2Controller instance /// method. D2ControllerTest() : DControllerTest(D2Controller::instance) { + file::PathChecker::enableEnforcement(false); + } + + /// @brief Destructor + virtual ~D2ControllerTest() { + file::PathChecker::enableEnforcement(true); } /// @brief Fetches the D2Controller's D2Process diff --git a/src/bin/d2/tests/d2_http_command_unittest.cc b/src/bin/d2/tests/d2_http_command_unittest.cc index ed243b15db..47b738ae83 100644 --- a/src/bin/d2/tests/d2_http_command_unittest.cc +++ b/src/bin/d2/tests/d2_http_command_unittest.cc @@ -19,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -36,6 +37,7 @@ using namespace isc::d2; using namespace isc::data; using namespace isc::http; using namespace isc::process; +using namespace isc::util; namespace ph = std::placeholders; namespace isc { @@ -105,6 +107,7 @@ public: /// Sets socket path to its default value. BaseCtrlChannelD2Test() : server_(NakedD2Controller::instance()) { + file::PathChecker::enableEnforcement(false); } /// @brief Destructor. @@ -118,6 +121,7 @@ public: // Reset command manager. CommandMgr::instance().deregisterAll(); HttpCommandMgr::instance().setConnectionTimeout(TIMEOUT_AGENT_RECEIVE_COMMAND); + file::PathChecker::enableEnforcement(true); } /// @brief Returns pointer to the server's IO service. diff --git a/src/bin/d2/tests/d2_process_unittests.cc b/src/bin/d2/tests/d2_process_unittests.cc index 315d68d8fd..e82f3eae08 100644 --- a/src/bin/d2/tests/d2_process_unittests.cc +++ b/src/bin/d2/tests/d2_process_unittests.cc @@ -15,6 +15,7 @@ #include #include #include +#include #include #include @@ -29,6 +30,7 @@ using namespace isc::d2; using namespace isc::data; using namespace isc::hooks; using namespace isc::process; +using namespace isc::util; using namespace boost::posix_time; namespace { @@ -71,12 +73,14 @@ public: HooksManager::setTestMode(false); D2Controller::instance(); init(); + file::PathChecker::enableEnforcement(false); } /// @brief Destructor virtual ~D2ProcessTest() { D2Controller::instance().reset(); resetHooksPath(); + file::PathChecker::enableEnforcement(true); } /// @brief Sets the Hooks path from which hooks can be loaded. diff --git a/src/bin/d2/tests/d2_simple_parser_unittest.cc b/src/bin/d2/tests/d2_simple_parser_unittest.cc index 51728cbfdc..0a6111a0b2 100644 --- a/src/bin/d2/tests/d2_simple_parser_unittest.cc +++ b/src/bin/d2/tests/d2_simple_parser_unittest.cc @@ -10,6 +10,7 @@ #include #include #include +#include #include #include @@ -18,6 +19,7 @@ using namespace isc; using namespace isc::data; using namespace isc::d2; using namespace isc::test; +using namespace isc::util; namespace { @@ -163,11 +165,13 @@ public: parser_type = D2ParserContext::PARSER_JSON) : parser_type_(parser_type) { reset(); + file::PathChecker::enableEnforcement(false); } /// @brief Destructor virtual ~D2SimpleParserTest() { reset(); + file::PathChecker::enableEnforcement(true); } /// @brief Parses JSON text and compares the results against an expected diff --git a/src/bin/d2/tests/get_config_unittest.cc b/src/bin/d2/tests/get_config_unittest.cc index c94589adea..f53f688a0d 100644 --- a/src/bin/d2/tests/get_config_unittest.cc +++ b/src/bin/d2/tests/get_config_unittest.cc @@ -153,6 +153,7 @@ public: resetConfiguration(); // Fill test secret file. fillSecretFile(); + file::PathChecker::enableEnforcement(false); } ~D2GetConfigTest() { @@ -160,6 +161,7 @@ public: resetConfiguration(); resetHooksPath(); resetSocketPath(); + file::PathChecker::enableEnforcement(true); } /// @brief Sets the Hooks path from which hooks can be loaded. diff --git a/src/bin/dhcp4/tests/config_parser_unittest.cc b/src/bin/dhcp4/tests/config_parser_unittest.cc index 51d66ec35e..3c21bfb07a 100644 --- a/src/bin/dhcp4/tests/config_parser_unittest.cc +++ b/src/bin/dhcp4/tests/config_parser_unittest.cc @@ -340,6 +340,7 @@ public: resetHooksPath(); Dhcpv4SrvTest::resetSocketPath(); + file::PathChecker::enableEnforcement(true); } ~Dhcp4ParserTest() { @@ -352,6 +353,7 @@ public: resetHooksPath(); Dhcpv4SrvTest::resetSocketPath(); + file::PathChecker::enableEnforcement(true); }; /// @brief Sets the Hooks path from which hooks can be loaded. @@ -6924,6 +6926,7 @@ TEST_F(Dhcp4ParserTest, hostsDatabases) { // This test checks comments. Please keep it last. TEST_F(Dhcp4ParserTest, comments) { Dhcpv4SrvTest::setSocketTestPath(); + file::PathChecker::enableEnforcement(false); string config = PARSER_CONFIGS[6]; extractConfig(config); diff --git a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc index 43bd2aa947..722cf3a7ee 100644 --- a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc +++ b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc @@ -2965,6 +2965,7 @@ class DBInitializer { void Dhcpv4SrvTest::checkConfigFiles() { setSocketTestPath(); + file::PathChecker::enableEnforcement(false); #if defined (HAVE_MYSQL) MySqlHostDataSourceInit mysql_init; #endif diff --git a/src/bin/dhcp4/tests/dhcp4_test_utils.cc b/src/bin/dhcp4/tests/dhcp4_test_utils.cc index 6c6dd1836b..d098994d22 100644 --- a/src/bin/dhcp4/tests/dhcp4_test_utils.cc +++ b/src/bin/dhcp4/tests/dhcp4_test_utils.cc @@ -61,6 +61,7 @@ BaseServerTest::~BaseServerTest() { Dhcpv4SrvTest::Dhcpv4SrvTest() : rcode_(-1), srv_(new NakedDhcpv4Srv(0)), multi_threading_(false), start_time_(PktEvent::now()) { + file::PathChecker::enableEnforcement(true); // Wipe any existing statistics isc::stats::StatsMgr::instance().removeAll(); @@ -92,6 +93,7 @@ Dhcpv4SrvTest::Dhcpv4SrvTest() Dhcpv4SrvTest::~Dhcpv4SrvTest() { resetSocketPath(); + file::PathChecker::enableEnforcement(true); // Make sure that we revert to default value CfgMgr::instance().clear(); diff --git a/src/bin/dhcp4/tests/get_config_unittest.cc b/src/bin/dhcp4/tests/get_config_unittest.cc index f93a1cf290..a1bba15802 100644 --- a/src/bin/dhcp4/tests/get_config_unittest.cc +++ b/src/bin/dhcp4/tests/get_config_unittest.cc @@ -16,6 +16,7 @@ #include #include #include +#include #include #include @@ -33,6 +34,7 @@ using namespace isc::data; using namespace isc::dhcp; using namespace isc::dhcp::test; using namespace isc::test; +using namespace isc::util; namespace { @@ -14575,12 +14577,14 @@ public: // Create fresh context. resetConfiguration(); Dhcpv4SrvTest::setSocketTestPath(); + file::PathChecker::enableEnforcement(false); } ~Dhcp4GetConfigTest() { resetConfiguration(); Dhcpv4SrvTest::resetSocketPath(); - }; + file::PathChecker::enableEnforcement(true); + } /// @brief Parse and Execute configuration /// diff --git a/src/bin/dhcp4/tests/get_config_unittest.cc.skel b/src/bin/dhcp4/tests/get_config_unittest.cc.skel index 5bcfae32fc..6db931d256 100644 --- a/src/bin/dhcp4/tests/get_config_unittest.cc.skel +++ b/src/bin/dhcp4/tests/get_config_unittest.cc.skel @@ -16,6 +16,7 @@ #include #include #include +#include #include #include @@ -33,6 +34,7 @@ using namespace isc::data; using namespace isc::dhcp; using namespace isc::dhcp::test; using namespace isc::test; +using namespace isc::util; namespace { @@ -172,11 +174,15 @@ public: srv_.reset(new ControlledDhcpv4Srv(0)); // Create fresh context. resetConfiguration(); + Dhcpv4SrvTest::setSocketTestPath(); + file::PathChecker::enableEnforcement(false); } ~Dhcp4GetConfigTest() { resetConfiguration(); - }; + Dhcpv4SrvTest::resetSocketPath(); + file::PathChecker::enableEnforcement(true); + } /// @brief Parse and Execute configuration /// diff --git a/src/bin/dhcp4/tests/http_control_socket_unittest.cc b/src/bin/dhcp4/tests/http_control_socket_unittest.cc index 8c3fb1c2d2..67126f8726 100644 --- a/src/bin/dhcp4/tests/http_control_socket_unittest.cc +++ b/src/bin/dhcp4/tests/http_control_socket_unittest.cc @@ -30,6 +30,7 @@ #include #include #include +#include #include "marker_file.h" @@ -102,6 +103,7 @@ public: IfaceMgr::instance().setDetectCallback(std::bind(&IfaceMgr::checkDetectIfaces, IfaceMgr::instancePtr().get(), ph::_1)); setLogTestPath("/dev"); + file::PathChecker::enableEnforcement(false); } /// @brief Destructor @@ -121,6 +123,7 @@ public: IfaceMgr::instance().closeSockets(); IfaceMgr::instance().detectIfaces(); resetLogPath(); + file::PathChecker::enableEnforcement(true); } /// @brief Sets the log path where log output may be written. diff --git a/src/bin/dhcp6/tests/config_parser_unittest.cc b/src/bin/dhcp6/tests/config_parser_unittest.cc index a531afd668..548c63fe92 100644 --- a/src/bin/dhcp6/tests/config_parser_unittest.cc +++ b/src/bin/dhcp6/tests/config_parser_unittest.cc @@ -38,6 +38,7 @@ #include #include #include +#include #include #include @@ -64,6 +65,7 @@ using namespace isc::dhcp; using namespace isc::dhcp::test; using namespace isc::hooks; using namespace isc::test; +using namespace isc::util; using namespace std; namespace { @@ -425,6 +427,7 @@ public: resetConfiguration(); resetHooksPath(); + file::PathChecker::enableEnforcement(true); } ~Dhcp6ParserTest() { @@ -436,6 +439,7 @@ public: static_cast(remove(UNLOAD_MARKER_FILE)); resetHooksPath(); + file::PathChecker::enableEnforcement(true); }; /// @brief Sets the Hooks path from which hooks can be loaded. @@ -7700,6 +7704,7 @@ TEST_F(Dhcp6ParserTest, hostsDatabases) { // This test checks comments. Please keep it last. TEST_F(Dhcp6ParserTest, comments) { setSocketTestPath(); + file::PathChecker::enableEnforcement(false); string config = PARSER_CONFIGS[9]; extractConfig(config); diff --git a/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc b/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc index a4f7159b9b..5a367b8b8c 100644 --- a/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc +++ b/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc @@ -386,6 +386,7 @@ class DBInitializer { void Dhcpv6SrvTest::checkConfigFiles() { setSocketTestPath(); + file::PathChecker::enableEnforcement(false); #if defined (HAVE_MYSQL) MySqlHostDataSourceInit mysql_init; #endif diff --git a/src/bin/dhcp6/tests/dhcp6_test_utils.cc b/src/bin/dhcp6/tests/dhcp6_test_utils.cc index 38da0847c3..7ea1fea27f 100644 --- a/src/bin/dhcp6/tests/dhcp6_test_utils.cc +++ b/src/bin/dhcp6/tests/dhcp6_test_utils.cc @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include @@ -44,6 +45,7 @@ BaseServerTest::BaseServerTest() { CfgMgr::instance().getDataDir(true, TEST_DATA_BUILDDIR); resetLogPath(); resetSocketPath(); + file::PathChecker::enableEnforcement(true); } BaseServerTest::~BaseServerTest() { @@ -64,6 +66,7 @@ BaseServerTest::~BaseServerTest() { isc::log::initLogger(); resetLogPath(); resetSocketPath(); + file::PathChecker::enableEnforcement(true); } void diff --git a/src/bin/dhcp6/tests/get_config_unittest.cc b/src/bin/dhcp6/tests/get_config_unittest.cc index b502e210ec..c3dfa1fb32 100644 --- a/src/bin/dhcp6/tests/get_config_unittest.cc +++ b/src/bin/dhcp6/tests/get_config_unittest.cc @@ -9,7 +9,6 @@ #include #include #include -#include #include #include #include @@ -14809,12 +14808,14 @@ public: // Reset configuration for each test. resetConfiguration(); BaseServerTest::setSocketTestPath(); + file::PathChecker::enableEnforcement(false); } ~Dhcp6GetConfigTest() { // Reset configuration database after each test. resetConfiguration(); BaseServerTest::resetSocketPath(); + file::PathChecker::enableEnforcement(true); }; /// @brief Parse and Execute configuration diff --git a/src/bin/dhcp6/tests/get_config_unittest.cc.skel b/src/bin/dhcp6/tests/get_config_unittest.cc.skel index a05a157eb1..ac7fd8182b 100644 --- a/src/bin/dhcp6/tests/get_config_unittest.cc.skel +++ b/src/bin/dhcp6/tests/get_config_unittest.cc.skel @@ -16,6 +16,7 @@ #include #include #include +#include #include #include @@ -33,6 +34,7 @@ using namespace isc::data; using namespace isc::dhcp; using namespace isc::dhcp::test; using namespace isc::test; +using namespace isc::util; namespace { @@ -171,11 +173,15 @@ public: // Reset configuration for each test. resetConfiguration(); + BaseServerTest::setSocketTestPath(); + file::PathChecker::enableEnforcement(false); } ~Dhcp6GetConfigTest() { // Reset configuration database after each test. resetConfiguration(); + BaseServerTest::resetSocketPath(); + file::PathChecker::enableEnforcement(true); }; /// @brief Parse and Execute configuration diff --git a/src/bin/dhcp6/tests/http_control_socket_unittest.cc b/src/bin/dhcp6/tests/http_control_socket_unittest.cc index 8bf8994dd0..b13634ca3c 100644 --- a/src/bin/dhcp6/tests/http_control_socket_unittest.cc +++ b/src/bin/dhcp6/tests/http_control_socket_unittest.cc @@ -92,6 +92,7 @@ public: : BaseServerTest() { reset(); setLogTestPath("/dev"); + file::PathChecker::enableEnforcement(false); } virtual ~HttpCtrlDhcpv6Test() { diff --git a/src/lib/config/tests/http_command_config_unittests.cc b/src/lib/config/tests/http_command_config_unittests.cc index 0e17e9dad2..82d092a230 100644 --- a/src/lib/config/tests/http_command_config_unittests.cc +++ b/src/lib/config/tests/http_command_config_unittests.cc @@ -298,6 +298,7 @@ TEST_F(HttpCommandConfigTest, headers) { // This test verifies a HTTP control socket configuration with authentication // can be parsed and unparsed. TEST_F(HttpCommandConfigTest, authentication) { + file::PathChecker::enableEnforcement(false); // Configure with authentication. string config = R"( { diff --git a/src/lib/d2srv/d2_config.cc b/src/lib/d2srv/d2_config.cc index 1a1726b871..1400dc3e05 100644 --- a/src/lib/d2srv/d2_config.cc +++ b/src/lib/d2srv/d2_config.cc @@ -21,6 +21,7 @@ using namespace isc::data; using namespace isc::process; +using namespace isc::util; namespace isc { namespace d2 { @@ -423,7 +424,16 @@ TSIGKeyInfoParser::parse(ConstElementPtr key_config) { } } else { secret = getString(key_config, "secret"); + if (file::PathChecker::shouldEnforceSecurity()) { + isc_throw(D2CfgError, "use of clear text TSIG 'secret' is NOT SECURE (" + << " (" << getPosition("secret", key_config) + << ")"); + } else { + LOG_WARN(dhcp_to_d2_logger, DHCP_DDNS_TSIG_SECRET_SECURITY_WARN) + .arg(getPosition("secret", key_config).str()); + } } + ConstElementPtr user_context = key_config->get("user-context"); // Algorithm must be valid. diff --git a/src/lib/d2srv/d2_messages.cc b/src/lib/d2srv/d2_messages.cc index 18892f39b7..16a34c1d8f 100644 --- a/src/lib/d2srv/d2_messages.cc +++ b/src/lib/d2srv/d2_messages.cc @@ -86,6 +86,7 @@ extern const isc::log::MessageID DHCP_DDNS_STARTED = "DHCP_DDNS_STARTED"; extern const isc::log::MessageID DHCP_DDNS_STARTING_TRANSACTION = "DHCP_DDNS_STARTING_TRANSACTION"; extern const isc::log::MessageID DHCP_DDNS_STATE_MODEL_UNEXPECTED_ERROR = "DHCP_DDNS_STATE_MODEL_UNEXPECTED_ERROR"; extern const isc::log::MessageID DHCP_DDNS_TRANS_SEND_ERROR = "DHCP_DDNS_TRANS_SEND_ERROR"; +extern const isc::log::MessageID DHCP_DDNS_TSIG_SECRET_SECURITY_WARN = "DHCP_DDNS_TSIG_SECRET_SECURITY_WARN"; extern const isc::log::MessageID DHCP_DDNS_UPDATE_REQUEST_SENT = "DHCP_DDNS_UPDATE_REQUEST_SENT"; extern const isc::log::MessageID DHCP_DDNS_UPDATE_RESPONSE_RECEIVED = "DHCP_DDNS_UPDATE_RESPONSE_RECEIVED"; @@ -174,6 +175,7 @@ const char* values[] = { "DHCP_DDNS_STARTING_TRANSACTION", "Request ID %1:", "DHCP_DDNS_STATE_MODEL_UNEXPECTED_ERROR", "Request ID %1: application encountered an unexpected error while carrying out a NameChangeRequest: %2", "DHCP_DDNS_TRANS_SEND_ERROR", "Request ID %1: application encountered an unexpected error while attempting to send a DNS update: %2", + "DHCP_DDNS_TSIG_SECRET_SECURITY_WARN", "use of clear text TSIG 'secret' is NOT SECURE: %1", "DHCP_DDNS_UPDATE_REQUEST_SENT", "Request ID %1: %2 to server: %3", "DHCP_DDNS_UPDATE_RESPONSE_RECEIVED", "Request ID %1: to server: %2 status: %3", NULL diff --git a/src/lib/d2srv/d2_messages.h b/src/lib/d2srv/d2_messages.h index b6a2b3f659..09434dc532 100644 --- a/src/lib/d2srv/d2_messages.h +++ b/src/lib/d2srv/d2_messages.h @@ -87,6 +87,7 @@ extern const isc::log::MessageID DHCP_DDNS_STARTED; extern const isc::log::MessageID DHCP_DDNS_STARTING_TRANSACTION; extern const isc::log::MessageID DHCP_DDNS_STATE_MODEL_UNEXPECTED_ERROR; extern const isc::log::MessageID DHCP_DDNS_TRANS_SEND_ERROR; +extern const isc::log::MessageID DHCP_DDNS_TSIG_SECRET_SECURITY_WARN; extern const isc::log::MessageID DHCP_DDNS_UPDATE_REQUEST_SENT; extern const isc::log::MessageID DHCP_DDNS_UPDATE_RESPONSE_RECEIVED; diff --git a/src/lib/d2srv/d2_messages.mes b/src/lib/d2srv/d2_messages.mes index 455fcdaa5b..2167064fd8 100644 --- a/src/lib/d2srv/d2_messages.mes +++ b/src/lib/d2srv/d2_messages.mes @@ -455,3 +455,17 @@ server. Logged at debug log level 50. This is a debug message issued when DHCP_DDNS receives sends a DNS update response from a DNS server. + +% DHCP_DDNS_SECURITY_CHECKS_DISABLED Invoked with command line option -X, Security checks are disabled!! +This warning is emitted when internal security checks normally +performed by kea-dhcp-ddns have been disabled via command line option '-X'. +This means the server is not enforcing restrictions on resource +paths or permissions. This mode of operation may expose your +environment to security vulnerabilities and should only be used +after consideration. + +% DHCP_DDNS_TSIG_SECRET_SECURITY_WARN use of clear text TSIG 'secret' is NOT SECURE: %1 +This warning message is issued when security enforcement is disabled +and TSIG key configuration uses clear text 'secret' rather +than 'secret-file'. The server will still use the key as configured +but is warning that doing so may pose a security risk. diff --git a/src/lib/http/auth_messages.cc b/src/lib/http/auth_messages.cc index 993ba8f998..bc85af8487 100644 --- a/src/lib/http/auth_messages.cc +++ b/src/lib/http/auth_messages.cc @@ -7,10 +7,12 @@ namespace isc { namespace http { +extern const isc::log::MessageID HTTP_CLIENT_PASSWORD_SECURITY_WARN = "HTTP_CLIENT_PASSWORD_SECURITY_WARN"; extern const isc::log::MessageID HTTP_CLIENT_REQUEST_AUTHORIZED = "HTTP_CLIENT_REQUEST_AUTHORIZED"; extern const isc::log::MessageID HTTP_CLIENT_REQUEST_BAD_AUTH_HEADER = "HTTP_CLIENT_REQUEST_BAD_AUTH_HEADER"; extern const isc::log::MessageID HTTP_CLIENT_REQUEST_NOT_AUTHORIZED = "HTTP_CLIENT_REQUEST_NOT_AUTHORIZED"; extern const isc::log::MessageID HTTP_CLIENT_REQUEST_NO_AUTH_HEADER = "HTTP_CLIENT_REQUEST_NO_AUTH_HEADER"; +extern const isc::log::MessageID HTTP_CLIENT_USER_SECURITY_WARN = "HTTP_CLIENT_USER_SECURITY_WARN"; } // namespace http } // namespace isc @@ -18,10 +20,12 @@ extern const isc::log::MessageID HTTP_CLIENT_REQUEST_NO_AUTH_HEADER = "HTTP_CLIE namespace { const char* values[] = { + "HTTP_CLIENT_PASSWORD_SECURITY_WARN", "use of clear text 'password' is NOT SECURE: %1", "HTTP_CLIENT_REQUEST_AUTHORIZED", "received HTTP request authorized for '%1'", "HTTP_CLIENT_REQUEST_BAD_AUTH_HEADER", "received HTTP request with malformed authentication header: %1", "HTTP_CLIENT_REQUEST_NOT_AUTHORIZED", "received HTTP request with not matching authentication header", "HTTP_CLIENT_REQUEST_NO_AUTH_HEADER", "received HTTP request without required authentication header", + "HTTP_CLIENT_USER_SECURITY_WARN", "use of clear text 'user' is NOT SECURE: %1", NULL }; diff --git a/src/lib/http/auth_messages.h b/src/lib/http/auth_messages.h index 96dde53e06..c12fb918cd 100644 --- a/src/lib/http/auth_messages.h +++ b/src/lib/http/auth_messages.h @@ -8,10 +8,12 @@ namespace isc { namespace http { +extern const isc::log::MessageID HTTP_CLIENT_PASSWORD_SECURITY_WARN; extern const isc::log::MessageID HTTP_CLIENT_REQUEST_AUTHORIZED; extern const isc::log::MessageID HTTP_CLIENT_REQUEST_BAD_AUTH_HEADER; extern const isc::log::MessageID HTTP_CLIENT_REQUEST_NOT_AUTHORIZED; extern const isc::log::MessageID HTTP_CLIENT_REQUEST_NO_AUTH_HEADER; +extern const isc::log::MessageID HTTP_CLIENT_USER_SECURITY_WARN; } // namespace http } // namespace isc diff --git a/src/lib/http/auth_messages.mes b/src/lib/http/auth_messages.mes index 685bdb3596..2e44e1e01a 100644 --- a/src/lib/http/auth_messages.mes +++ b/src/lib/http/auth_messages.mes @@ -22,3 +22,15 @@ provided incorrect user id and/or password. % HTTP_CLIENT_REQUEST_NO_AUTH_HEADER received HTTP request without required authentication header This information message is issued when the server receives a request without a required authentication header. + +% HTTP_CLIENT_PASSWORD_SECURITY_WARN use of clear text 'password' is NOT SECURE: %1 +This warning message is issued when security enforcement is disabled +and command socket configuration uses clear text 'password' rather +than 'password-file'. The server will still use the socket as configured +but is warning that doing so may pose a security risk. + +% HTTP_CLIENT_USER_SECURITY_WARN use of clear text 'user' is NOT SECURE: %1 +This warning message is issued when security enforcement is disabled +and command socket configuration uses clear text 'user' rather +than 'user-file'. The server will still use the socket as configured +but is warning that doing so may pose a security risk. diff --git a/src/lib/http/basic_auth_config.cc b/src/lib/http/basic_auth_config.cc index 6f6ff88649..0336b16d8c 100644 --- a/src/lib/http/basic_auth_config.cc +++ b/src/lib/http/basic_auth_config.cc @@ -232,6 +232,14 @@ BasicHttpAuthConfig::parse(const ConstElementPtr& config) { "password must not be a default one (" << password_cfg->getPosition() << ")"); } + + if (file::PathChecker::shouldEnforceSecurity()) { + isc_throw(DhcpConfigError, "use of clear text 'password' is NOT SECURE (" + << password_cfg->getPosition() << ")"); + } else { + LOG_INFO(auth_logger, HTTP_CLIENT_PASSWORD_SECURITY_WARN) + .arg(password_cfg->getPosition().str()); + } } // password file. @@ -286,6 +294,14 @@ BasicHttpAuthConfig::parse(const ConstElementPtr& config) { isc_throw(DhcpConfigError, "user must not contain a ':': '" << user << "' (" << user_cfg->getPosition() << ")"); } + + if (file::PathChecker::shouldEnforceSecurity()) { + isc_throw(DhcpConfigError, "use of clear text 'user' is NOT SECURE (" + << user_cfg->getPosition() << ")"); + } else { + LOG_INFO(auth_logger, HTTP_CLIENT_USER_SECURITY_WARN) + .arg(user_cfg->getPosition().str()); + } } // user file. diff --git a/src/lib/http/tests/basic_auth_config_unittests.cc b/src/lib/http/tests/basic_auth_config_unittests.cc index f53eac6de8..b13cc1ee5b 100644 --- a/src/lib/http/tests/basic_auth_config_unittests.cc +++ b/src/lib/http/tests/basic_auth_config_unittests.cc @@ -8,6 +8,8 @@ #include #include #include +#include +#include #include using namespace isc; @@ -15,6 +17,8 @@ using namespace isc::data; using namespace isc::dhcp; using namespace isc::http; using namespace isc::test; +using namespace isc::util; +using namespace isc::dhcp::test; using namespace std; namespace { @@ -86,8 +90,24 @@ TEST(BasicHttpAuthClientTest, basicOneFile) { runToElementTest(expected, client); } + +/// @brief Test fixture for testing BasicHttpAuthConfig. +class BasicHttpAuthConfigTest : public LogContentTest { +public: + + /// @brief Constructor. + BasicHttpAuthConfigTest() { + file::PathChecker::enableEnforcement(true); + } + + /// @brief Desstructor. + virtual ~BasicHttpAuthConfigTest() { + file::PathChecker::enableEnforcement(true); + } +}; + // Test that basic auth configuration works as expected. -TEST(BasicHttpAuthConfigTest, basic) { +TEST_F(BasicHttpAuthConfigTest, basic) { // Create a configuration. BasicHttpAuthConfig config; @@ -173,7 +193,7 @@ TEST(BasicHttpAuthConfigTest, basic) { } // Test that basic auth configuration with files works as expected. -TEST(BasicHttpAuthConfigTest, basicFiles) { +TEST_F(BasicHttpAuthConfigTest, basicFiles) { // Create a configuration. BasicHttpAuthConfig config; @@ -247,7 +267,8 @@ TEST(BasicHttpAuthConfigTest, basicFiles) { } // Test that basic auth configuration parses. -TEST(BasicHttpAuthConfigTest, parse) { +TEST_F(BasicHttpAuthConfigTest, parse) { + file::PathChecker::enableEnforcement(false); BasicHttpAuthConfig config; ElementPtr cfg; @@ -535,6 +556,7 @@ TEST(BasicHttpAuthConfigTest, parse) { cfg->set("clients", clients_cfg); EXPECT_NO_THROW(config.parse(cfg)); runToElementTest(cfg, config); + std::cout << "TKM config: " << prettyPrint(config.toElement()) << std::endl; // Check a working not empty config with files. config.clear(); @@ -547,6 +569,66 @@ TEST(BasicHttpAuthConfigTest, parse) { cfg->set("clients", clients_cfg); EXPECT_NO_THROW(config.parse(cfg)); runToElementTest(cfg, config); + + EXPECT_EQ(4, countFile("HTTP_CLIENT_PASSWORD_SECURITY_WARN" + " use of clear text 'password' is NOT SECURE: :0:0")); +} + +// Test that basic auth configuration catches use of 'user' +// and warns if security is disabled or throws if enabled. +TEST_F(BasicHttpAuthConfigTest, clearUser) { + BasicHttpAuthConfig auth; + + std::string json = R"( + { + "type": "basic", + "clients": [ + { + "user": "foo" + }] + } + )"; + + ConstElementPtr config; + ASSERT_NO_THROW_LOG(config = Element::fromJSON(json)); + ASSERT_THROW_MSG(auth.parse(config), DhcpConfigError, + "use of clear text 'user' is NOT SECURE (:6:21)"); + + // Disable security. + file::PathChecker::enableEnforcement(false); + ASSERT_NO_THROW_LOG(auth.parse(config)); + EXPECT_EQ(1, countFile("HTTP_CLIENT_USER_SECURITY_WARN " + "use of clear text 'user' is NOT SECURE: :6:21")); +} + +// Test that basic auth configuration catches use of 'password' +// and warns if security is disabled or throws if enabled. +TEST_F(BasicHttpAuthConfigTest, clearPassword) { + BasicHttpAuthConfig auth; + + std::string json = R"( + { + "type": "basic", + "clients": [ + { + "user": "foo", + "password": "bar" + }] + } + )"; + + ConstElementPtr config; + ASSERT_NO_THROW_LOG(config = Element::fromJSON(json)); + ASSERT_THROW_MSG(auth.parse(config), DhcpConfigError, + "use of clear text 'password' is NOT SECURE (:7:25)"); + + // Disable security. + file::PathChecker::enableEnforcement(false); + ASSERT_NO_THROW_LOG(auth.parse(config)); + EXPECT_EQ(1, countFile("HTTP_CLIENT_PASSWORD_SECURITY_WARN " + "use of clear text 'password' is NOT SECURE: :7:25")); + EXPECT_EQ(1, countFile("HTTP_CLIENT_USER_SECURITY_WARN " + "use of clear text 'user' is NOT SECURE: :6:21")); } } // end of anonymous namespace diff --git a/src/lib/testutils/dhcp_test_lib.sh.in b/src/lib/testutils/dhcp_test_lib.sh.in index b4f479d7d5..13565bd03f 100755 --- a/src/lib/testutils/dhcp_test_lib.sh.in +++ b/src/lib/testutils/dhcp_test_lib.sh.in @@ -701,8 +701,8 @@ start_kea() { test_lib_error "binary name must be specified for start_kea" clean_exit 1 fi - printf "Running command %s.\n" "\"${bin} -c ${CFG_FILE}\"" - "${bin}" -c "${CFG_FILE}" & + printf "Running command %s.\n" "\"${bin} -X -c ${CFG_FILE}\"" + "${bin}" -X -c "${CFG_FILE}" & } # Waits with timeout for Kea to start. @@ -1096,9 +1096,9 @@ password_redact_test() { # Instruct Control Agent to log to the specific file. set_logger # Check it - printf "Running command %s.\n" "\"${bin_path}/${bin} -d -t ${CFG_FILE}\"" + printf "Running command %s.\n" "\"${bin_path}/${bin} -X -d -t ${CFG_FILE}\"" run_command \ - "${bin_path}/${bin}" -d -t "${CFG_FILE}" + "${bin_path}/${bin}" -X -d -t "${CFG_FILE}" if [ "${EXIT_CODE}" -ne "${expected_code}" ]; then printf 'ERROR: expected exit code %s, got %s\n' "${expected_code}" "${EXIT_CODE}" clean_exit 1 -- 2.47.3