From d33ca01e44459f69f69c9db32a13c51722a549c0 Mon Sep 17 00:00:00 2001 From: Francis Dupont Date: Fri, 22 Nov 2024 11:24:15 +0100 Subject: [PATCH] [#3609] Addressed comments --- doc/sphinx/arm/agent.rst | 2 +- doc/sphinx/arm/ctrl-channel.rst | 16 ++++- doc/sphinx/arm/dhcp4-srv.rst | 2 +- doc/sphinx/arm/dhcp6-srv.rst | 2 +- src/bin/agent/ca_response_creator.cc | 72 +++++++++---------- src/bin/agent/simple_parser.cc | 2 +- src/bin/dhcp4/dhcp4_srv.cc | 2 +- src/bin/dhcp4/tests/config_parser_unittest.cc | 6 ++ src/bin/dhcp6/tests/config_parser_unittest.cc | 6 ++ src/lib/http/cfg_http_header.h | 7 ++ src/lib/http/header_context.h | 3 + src/lib/http/tests/response_unittests.cc | 2 +- 12 files changed, 78 insertions(+), 44 deletions(-) diff --git a/doc/sphinx/arm/agent.rst b/doc/sphinx/arm/agent.rst index 4862d46a92..3519cb097d 100644 --- a/doc/sphinx/arm/agent.rst +++ b/doc/sphinx/arm/agent.rst @@ -120,7 +120,7 @@ different from the HA peer URLs, which are strictly for internal HA traffic between the peers. User commands should still be sent via the CA. -Since Kea 1.7.5 the ``http-headers`` parameter specifies a list of +Since Kea 2.7.5 the ``http-headers`` parameter specifies a list of extra HTTP headers to add to HTTP responses. The ``trust-anchor``, ``cert-file``, ``key-file``, and ``cert-required`` diff --git a/doc/sphinx/arm/ctrl-channel.rst b/doc/sphinx/arm/ctrl-channel.rst index 2a1c2f6f4b..5940743586 100644 --- a/doc/sphinx/arm/ctrl-channel.rst +++ b/doc/sphinx/arm/ctrl-channel.rst @@ -210,7 +210,21 @@ depends on the specific command. .. note:: Since Kea 2.7.5 it is possible to specify extra HTTP headers which - are added to HTTP responses. + are added to HTTP responses. Each header is specified by its name + and value with optionally a user context. For instance: + +:: + + ..., + "http-headers": [ + { + "name": "Strict-Transport-Security", + "value": "max-age=31536000" + } + ], + ... + +adds a HSTS header declaring that HTTPS (vs HTTP) must be used for one year. .. _ctrl-channel-control-agent-command-response-format: diff --git a/doc/sphinx/arm/dhcp4-srv.rst b/doc/sphinx/arm/dhcp4-srv.rst index 50283a2408..cc46612479 100644 --- a/doc/sphinx/arm/dhcp4-srv.rst +++ b/doc/sphinx/arm/dhcp4-srv.rst @@ -7777,7 +7777,7 @@ TLS is required). The ``socket-address`` (default ``127.0.0.1``) and ``socket-port`` (default 8000) specify an IP address and port to which the HTTP service will be bound. -Since Kea 1.7.5 the ``http-headers`` parameter specifies a list of +Since Kea 2.7.5 the ``http-headers`` parameter specifies a list of extra HTTP headers to add to HTTP responses. The ``trust-anchor``, ``cert-file``, ``key-file``, and ``cert-required`` diff --git a/doc/sphinx/arm/dhcp6-srv.rst b/doc/sphinx/arm/dhcp6-srv.rst index 14c89c70eb..a1e0e8cf45 100644 --- a/doc/sphinx/arm/dhcp6-srv.rst +++ b/doc/sphinx/arm/dhcp6-srv.rst @@ -7591,7 +7591,7 @@ TLS is required). The ``socket-address`` (default ``::1``) and ``socket-port`` (default 8000) specify an IP address and port to which the HTTP service will be bound. -Since Kea 1.7.5 the ``http-headers`` parameter specifies a list of +Since Kea 2.7.5 the ``http-headers`` parameter specifies a list of extra HTTP headers to add to HTTP responses. The ``trust-anchor``, ``cert-file``, ``key-file``, and ``cert-required`` diff --git a/src/bin/agent/ca_response_creator.cc b/src/bin/agent/ca_response_creator.cc index ddc010acbf..ff0e3b5383 100644 --- a/src/bin/agent/ca_response_creator.cc +++ b/src/bin/agent/ca_response_creator.cc @@ -63,6 +63,31 @@ createStockHttpResponse(const HttpRequestPtr& request, return (response); } +namespace { + +/// Getting the config context. +CtrlAgentCfgContextPtr getCtrlAgentCfgContext() { + // There is a hierarchy of the objects through which we need to pass to get + // the configuration context. We may simplify this at some point but since + // we're in the singleton we want to make sure that we're using most current + // configuration. + CtrlAgentCfgContextPtr ctx; + boost::shared_ptr controller = + boost::dynamic_pointer_cast(CtrlAgentController::instance()); + if (controller) { + CtrlAgentProcessPtr process = controller->getCtrlAgentProcess(); + if (process) { + CtrlAgentCfgMgrPtr cfgmgr = process->getCtrlAgentCfgMgr(); + if (cfgmgr) { + ctx = cfgmgr->getCtrlAgentCfgContext(); + } + } + } + return (ctx); +} + +} // end of anonymous namespace. + HttpResponsePtr CtrlAgentResponseCreator:: createStockHttpResponseInternal(const HttpRequestPtr& request, @@ -81,20 +106,9 @@ createStockHttpResponseInternal(const HttpRequestPtr& request, } // This will generate the response holding JSON content. HttpResponsePtr response(new HttpResponseJson(http_version, status_code)); - // See the comment below. - boost::shared_ptr controller = - boost::dynamic_pointer_cast(CtrlAgentController::instance()); - if (controller) { - CtrlAgentProcessPtr process = controller->getCtrlAgentProcess(); - if (process) { - CtrlAgentCfgMgrPtr cfgmgr = process->getCtrlAgentCfgMgr(); - if (cfgmgr) { - CtrlAgentCfgContextPtr ctx = cfgmgr->getCtrlAgentCfgContext(); - if (ctx) { - copyHttpHeaders(ctx->getHttpHeaders(), *response); - } - } - } + CtrlAgentCfgContextPtr ctx = getCtrlAgentCfgContext(); + if (ctx) { + copyHttpHeaders(ctx->getHttpHeaders(), *response); } return (response); } @@ -109,29 +123,13 @@ createDynamicHttpResponse(HttpRequestPtr request) { HttpResponseJsonPtr http_response; // Context will hold the server configuration. - CtrlAgentCfgContextPtr ctx; - - // There is a hierarchy of the objects through which we need to pass to get - // the configuration context. We may simplify this at some point but since - // we're in the singleton we want to make sure that we're using most current - // configuration. - boost::shared_ptr controller = - boost::dynamic_pointer_cast(CtrlAgentController::instance()); - if (controller) { - CtrlAgentProcessPtr process = controller->getCtrlAgentProcess(); - if (process) { - CtrlAgentCfgMgrPtr cfgmgr = process->getCtrlAgentCfgMgr(); - if (cfgmgr) { - ctx = cfgmgr->getCtrlAgentCfgContext(); - if (ctx) { - headers = ctx->getHttpHeaders(); - const HttpAuthConfigPtr& auth = ctx->getAuthConfig(); - if (auth) { - // Check authentication. - http_response = auth->checkAuth(*this, request); - } - } - } + CtrlAgentCfgContextPtr ctx = getCtrlAgentCfgContext(); + if (ctx) { + headers = ctx->getHttpHeaders(); + const HttpAuthConfigPtr& auth = ctx->getAuthConfig(); + if (auth) { + // Check authentication. + http_response = auth->checkAuth(*this, request); } } diff --git a/src/bin/agent/simple_parser.cc b/src/bin/agent/simple_parser.cc index e3018730b8..b50cda1440 100644 --- a/src/bin/agent/simple_parser.cc +++ b/src/bin/agent/simple_parser.cc @@ -153,7 +153,7 @@ AgentSimpleParser::parse(const CtrlAgentCfgContextPtr& ctx, } } - // Basic HTTP authentications are forth. + // Basic HTTP authentications are fourth. ConstElementPtr auth_config = config->get("authentication"); if (auth_config) { using namespace isc::http; diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index bd2d56e8ab..8c85e98897 100644 --- a/src/bin/dhcp4/dhcp4_srv.cc +++ b/src/bin/dhcp4/dhcp4_srv.cc @@ -2894,7 +2894,7 @@ Dhcpv4Srv::createNameChangeRequests(const Lease4Ptr& lease, } if ((lease->reuseable_valid_lft_ == 0) && - (!old_lease || ddns_params.getUpdateOnRenew() || + (!old_lease || ddns_params.getUpdateOnRenew() || !lease->hasIdenticalFqdn(*old_lease))) { if (old_lease) { // Queue's up a remove of the old lease's DNS (if needed) diff --git a/src/bin/dhcp4/tests/config_parser_unittest.cc b/src/bin/dhcp4/tests/config_parser_unittest.cc index d178dce589..8ebfe37be8 100644 --- a/src/bin/dhcp4/tests/config_parser_unittest.cc +++ b/src/bin/dhcp4/tests/config_parser_unittest.cc @@ -7121,6 +7121,12 @@ TEST_F(Dhcp4ParserTest, comments) { ASSERT_EQ(1, headers->size()); ConstElementPtr header = headers->get(0); ASSERT_TRUE(header); + ASSERT_TRUE(header->get("name")); + EXPECT_EQ("\"Strict-Transport-Security\"", header->get("name")->str()); + ASSERT_TRUE(header->get("value")); + EXPECT_EQ("\"max-age=31536000\"", header->get("value")->str()); + + // Check HTTP header user context. ConstElementPtr ctx_header = header->get("user-context"); ASSERT_TRUE(ctx_header); ASSERT_EQ(1, ctx_header->size()); diff --git a/src/bin/dhcp6/tests/config_parser_unittest.cc b/src/bin/dhcp6/tests/config_parser_unittest.cc index c3f20bf3f4..51ba912d5d 100644 --- a/src/bin/dhcp6/tests/config_parser_unittest.cc +++ b/src/bin/dhcp6/tests/config_parser_unittest.cc @@ -7935,6 +7935,12 @@ TEST_F(Dhcp6ParserTest, comments) { ASSERT_EQ(1, headers->size()); ConstElementPtr header = headers->get(0); ASSERT_TRUE(header); + ASSERT_TRUE(header->get("name")); + EXPECT_EQ("\"Strict-Transport-Security\"", header->get("name")->str()); + ASSERT_TRUE(header->get("value")); + EXPECT_EQ("\"max-age=31536000\"", header->get("value")->str()); + + // Check HTTP header user context. ConstElementPtr ctx_header = header->get("user-context"); ASSERT_TRUE(ctx_header); ASSERT_EQ(1, ctx_header->size()); diff --git a/src/lib/http/cfg_http_header.h b/src/lib/http/cfg_http_header.h index ae3301e453..2b6e1993a2 100644 --- a/src/lib/http/cfg_http_header.h +++ b/src/lib/http/cfg_http_header.h @@ -17,9 +17,16 @@ namespace isc { namespace http { /// @brief Config HTTP header. +/// +/// Extra headers to include in a message are configured as a list of +/// objects of this class. At the difference of other HTTP header classes +/// there is no numeric value. class CfgHttpHeader : public isc::data::UserContext, public isc::data::CfgToElement { public: + /// @brief Header name. std::string name_; + + /// @brief Header value. std::string value_; /// @brief Constructor. diff --git a/src/lib/http/header_context.h b/src/lib/http/header_context.h index 9baf48201a..49819a3939 100644 --- a/src/lib/http/header_context.h +++ b/src/lib/http/header_context.h @@ -17,7 +17,10 @@ namespace http { /// @brief HTTP header context. struct HttpHeaderContext { + /// @brief Header name. std::string name_; + + /// @brief Header value. std::string value_; /// @brief Constructor. diff --git a/src/lib/http/tests/response_unittests.cc b/src/lib/http/tests/response_unittests.cc index f04a7ff74e..e60f20da89 100644 --- a/src/lib/http/tests/response_unittests.cc +++ b/src/lib/http/tests/response_unittests.cc @@ -180,7 +180,7 @@ TEST_F(HttpResponseTest, addHeader) { "Kea page title" "

Some header

" ""; - response.context()->headers_.push_back(HttpHeaderContext("Content-Type", "text/html")); + response.context()->headers_.push_back(HttpHeaderContext("Content-Type", "text/html")); response.context()->headers_.push_back(HttpHeaderContext("Host", "kea.example.org")); response.context()->body_ = sample_body; ASSERT_NO_THROW(response.finalize());