From: Thomas Markwalder Date: Tue, 10 Jun 2025 14:52:30 +0000 (-0400) Subject: [#3848] Throw or Warn if API sockets are unsecured X-Git-Tag: Kea-3.1.0~67 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=f8e9760eb7237059bacfaadf19f8db59099f7805;p=thirdparty%2Fkea.git [#3848] Throw or Warn if API sockets are unsecured /src/lib/config/config_messages.* COMMAND_HTTP_SOCKET_SECURITY_WARN - new message /src/lib/config/http_command_config.* HttpCommandConfig::HttpCommandConfig() - throw or warn when socket is unsecured HttpCommandConfig::checkTlsSetup() - return true if valid TLS is configured /src/lib/config/tests/http_command_config_unittests.cc /src/lib/config/tests/http_command_mgr_unittests.cc /src/lib/config/tests/http_command_response_creator_factory_unittests.cc /src/lib/config/tests/http_command_response_creator_unittests.cc Udpated tests --- diff --git a/src/lib/config/config_messages.cc b/src/lib/config/config_messages.cc index b6f76cfcd0..856965acc4 100644 --- a/src/lib/config/config_messages.cc +++ b/src/lib/config/config_messages.cc @@ -14,6 +14,7 @@ extern const isc::log::MessageID COMMAND_HTTP_LISTENER_COMMAND_REJECTED = "COMMA extern const isc::log::MessageID COMMAND_HTTP_LISTENER_STARTED = "COMMAND_HTTP_LISTENER_STARTED"; extern const isc::log::MessageID COMMAND_HTTP_LISTENER_STOPPED = "COMMAND_HTTP_LISTENER_STOPPED"; extern const isc::log::MessageID COMMAND_HTTP_LISTENER_STOPPING = "COMMAND_HTTP_LISTENER_STOPPING"; +extern const isc::log::MessageID COMMAND_HTTP_SOCKET_SECURITY_WARN = "COMMAND_HTTP_SOCKET_SECURITY_WARN"; extern const isc::log::MessageID COMMAND_PROCESS_ERROR1 = "COMMAND_PROCESS_ERROR1"; extern const isc::log::MessageID COMMAND_PROCESS_ERROR2 = "COMMAND_PROCESS_ERROR2"; extern const isc::log::MessageID COMMAND_RECEIVED = "COMMAND_RECEIVED"; @@ -56,6 +57,7 @@ const char* values[] = { "COMMAND_HTTP_LISTENER_STARTED", "Command HTTP listener started with %1 threads, listening on address: %2 port: %3, use TLS: %4", "COMMAND_HTTP_LISTENER_STOPPED", "Command HTTP listener for address: %1 port: %2 stopped.", "COMMAND_HTTP_LISTENER_STOPPING", "Stopping Command HTTP listener for address: %1 port: %2", + "COMMAND_HTTP_SOCKET_SECURITY_WARN", "command socket configuration is NOT SECURE: %1", "COMMAND_PROCESS_ERROR1", "Error while processing command: %1", "COMMAND_PROCESS_ERROR2", "Error while processing command: %1", "COMMAND_RECEIVED", "Received command '%1'", diff --git a/src/lib/config/config_messages.h b/src/lib/config/config_messages.h index 089aa1e57f..1edd106996 100644 --- a/src/lib/config/config_messages.h +++ b/src/lib/config/config_messages.h @@ -15,6 +15,7 @@ extern const isc::log::MessageID COMMAND_HTTP_LISTENER_COMMAND_REJECTED; extern const isc::log::MessageID COMMAND_HTTP_LISTENER_STARTED; extern const isc::log::MessageID COMMAND_HTTP_LISTENER_STOPPED; extern const isc::log::MessageID COMMAND_HTTP_LISTENER_STOPPING; +extern const isc::log::MessageID COMMAND_HTTP_SOCKET_SECURITY_WARN; extern const isc::log::MessageID COMMAND_PROCESS_ERROR1; extern const isc::log::MessageID COMMAND_PROCESS_ERROR2; extern const isc::log::MessageID COMMAND_RECEIVED; diff --git a/src/lib/config/config_messages.mes b/src/lib/config/config_messages.mes index dd7e733fe5..0492d28976 100644 --- a/src/lib/config/config_messages.mes +++ b/src/lib/config/config_messages.mes @@ -192,3 +192,9 @@ This warning message is issued when security enforcement is disabled and the path specified for a control channel unix socket-name does not have the required socket permissions. The server will still use the specified path but is warning that doing so may pose a security risk. + +% COMMAND_HTTP_SOCKET_SECURITY_WARN command socket configuration is NOT SECURE: %1 +This warning message is issued when security enforcement is disabled +and command socket configuration does not use HTTPS/TLS or baseic HTTP +authentication. 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/config/http_command_config.cc b/src/lib/config/http_command_config.cc index 02671cc1ad..ee3bd09a7f 100644 --- a/src/lib/config/http_command_config.cc +++ b/src/lib/config/http_command_config.cc @@ -8,8 +8,10 @@ #include #include +#include #include #include +#include #include using namespace isc; @@ -18,6 +20,7 @@ using namespace isc::config; using namespace isc::data; using namespace isc::dhcp; using namespace isc::http; +using namespace isc::util; using namespace std; namespace isc { @@ -174,7 +177,19 @@ HttpCommandConfig::HttpCommandConfig(ConstElementPtr config) } // Check the TLS setup. - checkTlsSetup(socket_type_ == "https"); + bool has_tls = checkTlsSetup(socket_type_ == "https"); + if (!auth_config_ && !has_tls) { + if (file::PathChecker::shouldEnforceSecurity()) { + isc_throw(DhcpConfigError, "Unsecured HTTP control channel (" + << config->getPosition() << ")"); + + } + + std::ostringstream oss; + config->toJSON(oss); + LOG_WARN(command_logger, COMMAND_HTTP_SOCKET_SECURITY_WARN) + .arg(oss.str()); + } // Get user context. ConstElementPtr user_context = config->get("user-context"); @@ -183,7 +198,7 @@ HttpCommandConfig::HttpCommandConfig(ConstElementPtr config) } } -void +bool HttpCommandConfig::checkTlsSetup(bool require_tls) const { bool have_ca = !trust_anchor_.empty(); bool have_cert = !cert_file_.empty(); @@ -193,7 +208,7 @@ HttpCommandConfig::checkTlsSetup(bool require_tls) const { isc_throw(DhcpConfigError, "no TLS setup for a HTTPS control socket"); } - return; + return (false); } // TLS is used: all 3 parameters are required. if (!have_ca) { @@ -209,6 +224,8 @@ HttpCommandConfig::checkTlsSetup(bool require_tls) const { isc_throw(DhcpConfigError, "key-file parameter is missing or empty:" " all or none of TLS parameters must be set"); } + + return (true); } ElementPtr diff --git a/src/lib/config/http_command_config.h b/src/lib/config/http_command_config.h index 0811823584..5dfc0e2cd2 100644 --- a/src/lib/config/http_command_config.h +++ b/src/lib/config/http_command_config.h @@ -196,7 +196,13 @@ private: /// @brief Check TLS configuration. /// /// @param require_tls When true TLS is required. - void checkTlsSetup(bool require_tls) const; + /// @return true if TLS parameters are all present, false + /// otherwise. + /// @throw DhcpConfigError if socket type is https and + /// TLS is missing or incomplete, or if socket type is + /// http and some TLS parameters are specified but not + /// all. + bool checkTlsSetup(bool require_tls) const; /// @brief Socket type ("http" or "https"). std::string socket_type_; diff --git a/src/lib/config/tests/http_command_config_unittests.cc b/src/lib/config/tests/http_command_config_unittests.cc index 493276d9e4..0e17e9dad2 100644 --- a/src/lib/config/tests/http_command_config_unittests.cc +++ b/src/lib/config/tests/http_command_config_unittests.cc @@ -8,8 +8,10 @@ #include #include +#include #include #include +#include using namespace isc; using namespace isc::asiolink; @@ -18,18 +20,21 @@ 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 { /// @brief Test fixture for HTTP control socket configuration. -class HttpCommandConfigTest : public ::testing::Test { +class HttpCommandConfigTest : public LogContentTest { public: /// @brief Constructor. HttpCommandConfigTest() : http_config_() { HttpCommandConfig::DEFAULT_SOCKET_ADDRESS = IOAddress("127.0.0.1"); HttpCommandConfig::DEFAULT_SOCKET_PORT = 8000; HttpCommandConfig::DEFAULT_AUTHENTICATION_REALM = ""; + file::PathChecker::enableEnforcement(true); } /// @brief Destructor. @@ -37,6 +42,7 @@ public: HttpCommandConfig::DEFAULT_SOCKET_ADDRESS = IOAddress("127.0.0.1"); HttpCommandConfig::DEFAULT_SOCKET_PORT = 8000; HttpCommandConfig::DEFAULT_AUTHENTICATION_REALM = ""; + file::PathChecker::enableEnforcement(true); } /// @brief HTTP control socket configuration. @@ -45,7 +51,14 @@ public: // This test verifies the default HTTP control socket configuration. TEST_F(HttpCommandConfigTest, default) { + // "Default" config should throw since security is enabled. ElementPtr json = Element::createMap(); + ASSERT_THROW_MSG(http_config_.reset(new HttpCommandConfig(json)), + DhcpConfigError, "Unsecured HTTP control channel (:0:0)"); + + // Turn off security enforcment. Configuration will succeed but we + // should see WARN logs. + file::PathChecker::enableEnforcement(false); ASSERT_NO_THROW_LOG(http_config_.reset(new HttpCommandConfig(json))); // Check default values. @@ -70,12 +83,18 @@ TEST_F(HttpCommandConfigTest, default) { )"; runToElementTest(expected, *http_config_); + ASSERT_EQ(1, countFile("COMMAND_HTTP_SOCKET_SECURITY_WARN command socket" + " configuration is NOT SECURE: { }")); + // Change class defaults. HttpCommandConfig::DEFAULT_SOCKET_ADDRESS = IOAddress("::1"); HttpCommandConfig::DEFAULT_SOCKET_PORT = 8080; ASSERT_NO_THROW_LOG(http_config_.reset(new HttpCommandConfig(json))); EXPECT_EQ("::1", http_config_->getSocketAddress().toText()); EXPECT_EQ(8080, http_config_->getSocketPort()); + + ASSERT_EQ(2, countFile("COMMAND_HTTP_SOCKET_SECURITY_WARN command socket" + " configuration is NOT SECURE: { }")); } // This test verifies direct error cases. @@ -221,6 +240,8 @@ TEST_F(HttpCommandConfigTest, errors) { // This test verifies a HTTP control socket configuration with HTTP headers // can be parsed and unparsed. TEST_F(HttpCommandConfigTest, headers) { + // Disable security enforcement. + file::PathChecker::enableEnforcement(false); // Configure with HTTP headers. string config = R"( { @@ -265,6 +286,13 @@ TEST_F(HttpCommandConfigTest, headers) { ] })"; runToElementTest(expected, *http_config_); + + // Should have security WARN log. + ASSERT_EQ(1, countFile("COMMAND_HTTP_SOCKET_SECURITY_WARN command socket" + " configuration is NOT SECURE: { \"http-headers\": " + "[ { \"name\": \"Strict-Transport-Security\", \"value\":" + " \"max-age=31536000\" }, { \"name\": \"Foo\", \"value\": \"bar\"" + " } ] }")); } // This test verifies a HTTP control socket configuration with authentication @@ -334,6 +362,9 @@ TEST_F(HttpCommandConfigTest, authentication) { } })"; runToElementTest(expected, *http_config_); + + // Should be no security warnings. + ASSERT_EQ(0, countFile("COMMAND_HTTP_SOCKET_SECURITY_WARN")); } // This test verifies a HTTP control socket configuration with TLS can diff --git a/src/lib/config/tests/http_command_mgr_unittests.cc b/src/lib/config/tests/http_command_mgr_unittests.cc index 6606fe2a02..691483061c 100644 --- a/src/lib/config/tests/http_command_mgr_unittests.cc +++ b/src/lib/config/tests/http_command_mgr_unittests.cc @@ -16,6 +16,7 @@ #include #include #include +#include #include #include @@ -57,6 +58,7 @@ public: HttpCommandMgrTest() : io_service_(new IOService()), test_timer_(io_service_), client_(), http_config_() { + file::PathChecker::enableEnforcement(false); resetState(io_service_); test_timer_.setup(std::bind(&HttpCommandMgrTest::timeoutHandler, this, true), TEST_TIMEOUT, IntervalTimer::ONE_SHOT); @@ -83,6 +85,7 @@ public: resetState(); IfaceMgr::instance().deleteAllExternalSockets(); io_service_->stopAndPoll(); + file::PathChecker::enableEnforcement(true); } /// @brief Resets state. diff --git a/src/lib/config/tests/http_command_response_creator_factory_unittests.cc b/src/lib/config/tests/http_command_response_creator_factory_unittests.cc index 953d8f72c5..ca656820a4 100644 --- a/src/lib/config/tests/http_command_response_creator_factory_unittests.cc +++ b/src/lib/config/tests/http_command_response_creator_factory_unittests.cc @@ -7,19 +7,35 @@ #include #include +#include #include #include using namespace isc::config; using namespace isc::data; +using namespace isc::util; using namespace std; namespace { +/// @brief Test fixture class for @ref class HttpCommandResponseCreatorFactory +class HttpCommandResponseCreatorFactoryTest : public ::testing::Test { +public: + /// @brief Constructor. + HttpCommandResponseCreatorFactoryTest() { + file::PathChecker::enableEnforcement(false); + } + + /// @brief Desstructor. + virtual ~HttpCommandResponseCreatorFactoryTest() { + file::PathChecker::enableEnforcement(true); + } +}; + // This test verifies the default factory constructor and // the create() method. -TEST(HttpCommandResponseCreatorFactory, create) { +TEST_F(HttpCommandResponseCreatorFactoryTest, create) { // Configure the HTTP control socket. string config = R"( { diff --git a/src/lib/config/tests/http_command_response_creator_unittests.cc b/src/lib/config/tests/http_command_response_creator_unittests.cc index 45022465a9..ed9d90b24d 100644 --- a/src/lib/config/tests/http_command_response_creator_unittests.cc +++ b/src/lib/config/tests/http_command_response_creator_unittests.cc @@ -13,6 +13,7 @@ #include #include #include +#include #include #include @@ -22,6 +23,7 @@ using namespace isc; using namespace isc::config; using namespace isc::data; using namespace isc::http; +using namespace isc::util; using namespace std; namespace ph = std::placeholders; @@ -46,6 +48,7 @@ public: /// create "empty" request. It also removes registered commands from the /// command manager. HttpCommandResponseCreatorTest() { + file::PathChecker::enableEnforcement(false); // Deregisters commands. config::CommandMgr::instance().deregisterAll(); // Register our "foo" command. @@ -59,6 +62,7 @@ public: /// Removes registered commands from the command manager. virtual ~HttpCommandResponseCreatorTest() { config::CommandMgr::instance().deregisterAll(); + file::PathChecker::enableEnforcement(true); } /// @brief Create HTTP control socket configuration (from text).