From 1d7c894188b5fe41e23c05fafcf0df6f9ef7db71 Mon Sep 17 00:00:00 2001 From: Francis Dupont Date: Fri, 15 Dec 2017 00:53:20 +0100 Subject: [PATCH] [5443] Checkpoint: proposed code for dhcp4, todo new tests and dhcp6 port --- src/bin/dhcp4/dhcp4_hooks.dox | 24 ++++--- src/bin/dhcp4/dhcp4_messages.mes | 19 ++++- src/bin/dhcp4/dhcp4_srv.cc | 93 ++++++++++++++++++------- src/bin/dhcp4/dhcp4_srv.h | 12 ++-- src/bin/dhcp4/tests/dhcp4_test_utils.cc | 2 +- src/bin/dhcp4/tests/dhcp4_test_utils.h | 2 +- src/bin/dhcp4/tests/fqdn_unittest.cc | 4 +- 7 files changed, 111 insertions(+), 45 deletions(-) diff --git a/src/bin/dhcp4/dhcp4_hooks.dox b/src/bin/dhcp4/dhcp4_hooks.dox index 9c87a66c2d..c522453e6a 100644 --- a/src/bin/dhcp4/dhcp4_hooks.dox +++ b/src/bin/dhcp4/dhcp4_hooks.dox @@ -61,15 +61,15 @@ to the end of this list. are set yet. Callouts installed on this hook point can modify the data in the received buffer. The server will parse the buffer afterwards. - - Next step status: If any callout sets the status to SKIP, the server will + - Next step status: If any callout sets the status to DROP, the server + will drop the packet and start processing the next one. + If any callout sets the status to SKIP, the server will skip the buffer parsing. In this case there is an expectation that the callout will parse the options carried in the buffer, create @c isc::dhcp::Option objects (or derived) and add them to the "query4" object using the @c isc::dhcp::Pkt4::addOption. Otherwise the server will find out that some mandatory options are - missing (e.g. DHCP Message Type) and will drop the message. If you - want to have the capability to drop a message, it is better to use - the skip flag in the "pkt4_receive" callout. + missing (e.g. DHCP Message Type) and will drop the message. @subsection dhcpv4HooksPkt4Receive pkt4_receive @@ -87,7 +87,7 @@ to the end of this list. field has been already parsed and stored in other fields. Therefore, the modification in the data_ field has no effect. - - Next step status: If any callout sets the status to SKIP, the server will + - Next step status: If any callout sets the status to SKIP (or DROP), the server will drop the packet and start processing the next one. The reason for the drop will be logged if logging is set to the appropriate debug level. @@ -107,7 +107,9 @@ to the end of this list. not be modified. - Next step status: If any callout installed on the "subnet4_select" hook - sets the next step status to SKIP, the server will not select any subnet. + sets the next step status to DROP, the server cancels current processing, + drop the packet and start processing the next one. + If any callout sets the status to SKIP, the server will not select any subnet. Packet processing will continue, but will be severely limited. @subsection dhcpv4HooksHost4Identifier host4_identifier @@ -186,7 +188,7 @@ to the end of this list. be released. It doesn't make sense to modify it at this time. - Next step status: If any callout installed on the "lease4_release" hook - sets the next step action to SKIP, the server will not delete the lease. + sets the next step action to SKIP (or DROP), the server will not delete the lease. It will be kept in the database and will go through the regular expiration/reuse process. @@ -206,8 +208,8 @@ to the end of this list. modify it at this time. All the information will be removed from the lease before it is updated in the database. - - Next step status: If any callout installed on the "lease4_release" hook - sets the next step action to DROP, the server will not decline the lease. + - Next step status: If any callout installed on the "lease4_decline" hook + sets the next step action to SKIP (or DROP), the server will not decline the lease. Care should be taken when setting this status. The lease will be kept in the database as it is and the client will incorrectly assume that the server marked this lease as unavailable. If the client restarts its configuration, @@ -233,7 +235,7 @@ to the end of this list. cannot be modified. - Next step action: if any callout installed on the "pkt4_send" hook - sets the next step action to SKIP, the server will not construct the raw + sets the next step action to SKIP (or DROP), the server will not construct the raw buffer. The expectation is that if the callout set skip flag, it is responsible for constructing raw form on its own. Otherwise the output packet will be sent with zero length. @@ -254,7 +256,7 @@ to the end of this list. time, because they were already used to construct on-wire buffer. Their modification would have no effect. - - Next step status: if any callout sets the next step action to SKIP, + - Next step status: if any callout sets the next step action to SKIP (or DROP), the server will drop this response packet. However, the original request packet from a client was processed, so server's state most likely has changed (e.g. lease was allocated). Setting this flag merely stops the change diff --git a/src/bin/dhcp4/dhcp4_messages.mes b/src/bin/dhcp4/dhcp4_messages.mes index 05426d29e7..c6a7848400 100644 --- a/src/bin/dhcp4/dhcp4_messages.mes +++ b/src/bin/dhcp4/dhcp4_messages.mes @@ -238,13 +238,21 @@ A "libreload" command was issued to reload the hooks libraries but for some reason the reload failed. Other error messages issued from the hooks framework will indicate the nature of the problem. -% DHCP4_HOOK_BUFFER_RCVD_SKIP received buffer from %1 to %2 over interface %3 was dropped because a callout set the skip flag +% DHCP4_HOOK_BUFFER_RCVD_DROP received buffer from %1 to %2 over interface %3 was dropped because a callout set the drop flag This debug message is printed when a callout installed on buffer4_receive -hook point set the skip flag. For this particular hook point, the +hook point set the drop flag. For this particular hook point, the setting of the flag by a callout instructs the server to drop the packet. The arguments specify the source and destination IPv4 address as well as the name of the interface over which the buffer has been received. +% DHCP4_HOOK_BUFFER_RCVD_SKIP received buffer from %1 to %2 over interface %3 is not parsed because a callout set the skip flag +This debug message is printed when a callout installed on +buffer4_receive hook point set the skip flag. For this particular hook +point, the setting of the flag by a callout instructs the server to +not parse the buffer because it was already parsed by the hook. The +arguments specify the source and destination IPv4 address as well as +the name of the interface over which the buffer has been received. + % DHCP4_HOOK_BUFFER_SEND_SKIP %1: prepared response is dropped because a callout set the skip flag. This debug message is printed when a callout installed on buffer4_send hook point set the skip flag. For this particular hook point, the @@ -277,6 +285,13 @@ of the flag instructs the server to drop the packet. This means that the client will not get any response, even though the server processed client's request and acted on it (e.g. possibly allocated a lease). +% DHCP4_HOOK_SUBNET4_SELECT_DROP %1: packet was dropped, because a callout set drop flag +This debug message is printed when a callout installed on the +subnet4_select hook point sets the drop flag. For this particular hook +point, the setting of the flag instructs the server to drop the receive +packet. The argument specifies the client and transaction identification +information. + % DHCP4_HOOK_SUBNET4_SELECT_SKIP %1: no subnet was selected, because a callout set skip flag. This debug message is printed when a callout installed on the subnet4_select hook point sets the skip flag. For this particular hook diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index 97ecfa5cf3..9a12a33b6a 100644 --- a/src/bin/dhcp4/dhcp4_srv.cc +++ b/src/bin/dhcp4/dhcp4_srv.cc @@ -483,7 +483,7 @@ Dhcpv4Srv::shutdown() { } isc::dhcp::Subnet4Ptr -Dhcpv4Srv::selectSubnet(const Pkt4Ptr& query) const { +Dhcpv4Srv::selectSubnet(Pkt4Ptr& query) { // DHCPv4-over-DHCPv6 is a special (and complex) case if (query->isDhcp4o6()) { @@ -569,7 +569,16 @@ Dhcpv4Srv::selectSubnet(const Pkt4Ptr& query) const { return (Subnet4Ptr()); } - /// @todo: Add support for DROP status + // Callouts decided to drop the packet. It is a superset of the + // skip case so no subnet will be selected. For the caller to + // know it has to drop the packet the query pointer is cleared. + if (callout_handle->getStatus() == CalloutHandle::NEXT_STEP_DROP) { + LOG_DEBUG(hooks_logger, DBG_DHCP4_HOOKS, + DHCP4_HOOK_SUBNET4_SELECT_DROP) + .arg(query->getLabel()); + query.reset(); + return (Subnet4Ptr()); + } // Use whatever subnet was specified by the callout callout_handle->getArgument("subnet4", subnet); @@ -596,7 +605,7 @@ Dhcpv4Srv::selectSubnet(const Pkt4Ptr& query) const { } isc::dhcp::Subnet4Ptr -Dhcpv4Srv::selectSubnet4o6(const Pkt4Ptr& query) const { +Dhcpv4Srv::selectSubnet4o6(Pkt4Ptr& query) { Subnet4Ptr subnet; @@ -673,7 +682,16 @@ Dhcpv4Srv::selectSubnet4o6(const Pkt4Ptr& query) const { return (Subnet4Ptr()); } - /// @todo: Add support for DROP status + // Callouts decided to drop the packet. It is a superset of the + // skip case so no subnet will be selected. For the caller to + // know it has to drop the packet the query pointer is cleared. + if (callout_handle->getStatus() == CalloutHandle::NEXT_STEP_DROP) { + LOG_DEBUG(hooks_logger, DBG_DHCP4_HOOKS, + DHCP4_HOOK_SUBNET4_SELECT_DROP) + .arg(query->getLabel()); + query.reset(); + return (Subnet4Ptr()); + } // Use whatever subnet was specified by the callout callout_handle->getArgument("subnet4", subnet); @@ -841,15 +859,14 @@ Dhcpv4Srv::run_one() { // Callouts decided to skip the next processing step. The next // processing step would to parse the packet, so skip at this // stage means drop. - if (callout_handle->getStatus() == CalloutHandle::NEXT_STEP_SKIP) { + if ((callout_handle->getStatus() == CalloutHandle::NEXT_STEP_SKIP) || + (callout_handle->getStatus() == CalloutHandle::NEXT_STEP_DROP)) { LOG_DEBUG(hooks_logger, DBG_DHCP4_HOOKS, DHCP4_HOOK_BUFFER_SEND_SKIP) .arg(rsp->getLabel()); return; } - /// @todo: Add support for DROP status. - callout_handle->getArgument("response4", rsp); } @@ -911,6 +928,18 @@ Dhcpv4Srv::processPacket(Pkt4Ptr& query, Pkt4Ptr& rsp) { HooksManager::callCallouts(Hooks.hook_index_buffer4_receive_, *callout_handle); + // Callouts decided to drop the received packet. + // The response (rsp) is null so the caller (run_one) will + // immediately return too. + if (callout_handle->getStatus() == CalloutHandle::NEXT_STEP_DROP) { + LOG_DEBUG(hooks_logger, DBG_DHCP4_DETAIL, + DHCP4_HOOK_BUFFER_RCVD_DROP) + .arg(query->getRemoteAddr().toText()) + .arg(query->getLocalAddr().toText()) + .arg(query->getIface()); + return; + } + // Callouts decided to skip the next processing step. The next // processing step would to parse the packet, so skip at this // stage means that callouts did the parsing already, so server @@ -925,8 +954,6 @@ Dhcpv4Srv::processPacket(Pkt4Ptr& query, Pkt4Ptr& rsp) { } callout_handle->getArgument("query4", query); - - /// @todo: add support for DROP status } // Unpack the packet information unless the buffer4_receive callouts @@ -1010,15 +1037,14 @@ Dhcpv4Srv::processPacket(Pkt4Ptr& query, Pkt4Ptr& rsp) { // Callouts decided to skip the next processing step. The next // processing step would to process the packet, so skip at this // stage means drop. - if (callout_handle->getStatus() == CalloutHandle::NEXT_STEP_SKIP) { + if ((callout_handle->getStatus() == CalloutHandle::NEXT_STEP_SKIP) || + (callout_handle->getStatus() == CalloutHandle::NEXT_STEP_DROP)) { LOG_DEBUG(hooks_logger, DBG_DHCP4_HOOKS, DHCP4_HOOK_PACKET_RCVD_SKIP) .arg(query->getLabel()); return; } - /// @todo: Add support for DROP status - callout_handle->getArgument("query4", query); } @@ -1106,14 +1132,13 @@ Dhcpv4Srv::processPacket(Pkt4Ptr& query, Pkt4Ptr& rsp) { // Callouts decided to skip the next processing step. The next // processing step would to send the packet, so skip at this // stage means "drop response". - if (callout_handle->getStatus() == CalloutHandle::NEXT_STEP_SKIP) { + if ((callout_handle->getStatus() == CalloutHandle::NEXT_STEP_SKIP) || + (callout_handle->getStatus() == CalloutHandle::NEXT_STEP_DROP)) { LOG_DEBUG(hooks_logger, DBG_DHCP4_HOOKS, DHCP4_HOOK_PACKET_SEND_SKIP) .arg(query->getLabel()); skip_pack = true; } - - /// @todo: Add support for DROP status } if (!skip_pack) { @@ -2318,6 +2343,11 @@ Dhcpv4Srv::processDiscover(Pkt4Ptr& discover) { Dhcpv4Exchange ex(alloc_engine_, discover, selectSubnet(discover)); + // Stop here if selectSubnet decided to drop the packet + if (!discover) { + return (Pkt4Ptr()); + } + // If DHCPDISCOVER message contains the FQDN or Hostname option, server // may respond to the client with the appropriate FQDN or Hostname // option to indicate that whether it will take responsibility for @@ -2370,6 +2400,11 @@ Dhcpv4Srv::processRequest(Pkt4Ptr& request) { Dhcpv4Exchange ex(alloc_engine_, request, selectSubnet(request)); + // Stop here if selectSubnet decided to drop the packet + if (!request) { + return (Pkt4Ptr()); + } + // If DHCPREQUEST message contains the FQDN or Hostname option, server // should respond to the client with the appropriate FQDN or Hostname // option to indicate if it takes responsibility for the DNS updates. @@ -2478,14 +2513,13 @@ Dhcpv4Srv::processRelease(Pkt4Ptr& release) { // Callouts decided to skip the next processing step. The next // processing step would to send the packet, so skip at this // stage means "drop response". - if (callout_handle->getStatus() == CalloutHandle::NEXT_STEP_SKIP) { + if ((callout_handle->getStatus() == CalloutHandle::NEXT_STEP_SKIP) || + (callout_handle->getStatus() == CalloutHandle::NEXT_STEP_DROP)) { skip = true; LOG_DEBUG(hooks_logger, DBG_DHCP4_HOOKS, DHCP4_HOOK_LEASE4_RELEASE_SKIP) .arg(release->getLabel()); } - - /// @todo add support for DROP status } // Callout didn't indicate to skip the release process. Let's release @@ -2619,9 +2653,10 @@ Dhcpv4Srv::declineLease(const Lease4Ptr& lease, const Pkt4Ptr& decline) { HooksManager::callCallouts(Hooks.hook_index_lease4_decline_, *callout_handle); - // Check if callouts decided to drop the packet. If any of them did, - // we will drop the packet. - if (callout_handle->getStatus() == CalloutHandle::NEXT_STEP_DROP) { + // Check if callouts decided to skip the next processing step. + // If any of them did, we will drop the packet. + if ((callout_handle->getStatus() == CalloutHandle::NEXT_STEP_SKIP) || + (callout_handle->getStatus() == CalloutHandle::NEXT_STEP_DROP)) { LOG_DEBUG(hooks_logger, DBG_DHCP4_HOOKS, DHCP4_HOOK_DECLINE_SKIP) .arg(decline->getLabel()).arg(lease->addr_.toText()); return; @@ -2671,6 +2706,11 @@ Dhcpv4Srv::processInform(Pkt4Ptr& inform) { Dhcpv4Exchange ex(alloc_engine_, inform, selectSubnet(inform)); + // Stop here if selectSubnet decided to drop the packet + if (!inform) { + return (Pkt4Ptr()); + } + Pkt4Ptr ack = ex.getResponse(); ex.setReservedClientClasses(); @@ -2707,7 +2747,7 @@ Dhcpv4Srv::processInform(Pkt4Ptr& inform) { } bool -Dhcpv4Srv::accept(const Pkt4Ptr& query) const { +Dhcpv4Srv::accept(Pkt4Ptr& query) { // Check that the message type is accepted by the server. We rely on the // function called to log a message if needed. if (!acceptMessageType(query)) { @@ -2735,7 +2775,7 @@ Dhcpv4Srv::accept(const Pkt4Ptr& query) const { } bool -Dhcpv4Srv::acceptDirectRequest(const Pkt4Ptr& pkt) const { +Dhcpv4Srv::acceptDirectRequest(Pkt4Ptr& pkt) { // Accept all relayed messages. if (pkt->isRelayed()) { return (true); @@ -2762,7 +2802,12 @@ Dhcpv4Srv::acceptDirectRequest(const Pkt4Ptr& pkt) const { // we validate the message type prior to calling this function. return (false); } - return (!pkt->getLocalAddr().isV4Bcast() || selectSubnet(pkt)); + bool result = (!pkt->getLocalAddr().isV4Bcast() || selectSubnet(pkt)); + if (!pkt) { + // The packet must be dropped. + return (false); + } + return (result); } bool diff --git a/src/bin/dhcp4/dhcp4_srv.h b/src/bin/dhcp4/dhcp4_srv.h index e72af87c84..d601ddddbe 100644 --- a/src/bin/dhcp4/dhcp4_srv.h +++ b/src/bin/dhcp4/dhcp4_srv.h @@ -348,7 +348,7 @@ protected: /// /// @return true if the message should be further processed, or false if /// the message should be discarded. - bool accept(const Pkt4Ptr& query) const; + bool accept(Pkt4Ptr& query); /// @brief Check if a message sent by directly connected client should be /// accepted or discarded. @@ -377,7 +377,7 @@ protected: /// /// @return true if message is accepted for further processing, false /// otherwise. - bool acceptDirectRequest(const Pkt4Ptr& query) const; + bool acceptDirectRequest(Pkt4Ptr& query); /// @brief Check if received message type is valid for the server to /// process. @@ -766,15 +766,19 @@ protected: /// @brief Selects a subnet for a given client's packet. /// + /// When the packet has to be dropped the query pointer is cleared + /// /// @param query client's message /// @return selected subnet (or NULL if no suitable subnet was found) - isc::dhcp::Subnet4Ptr selectSubnet(const Pkt4Ptr& query) const; + isc::dhcp::Subnet4Ptr selectSubnet(Pkt4Ptr& query); /// @brief Selects a subnet for a given client's DHCP4o6 packet. /// + /// When the packet has to be dropped the query pointer is cleared + /// /// @param query client's message /// @return selected subnet (or NULL if no suitable subnet was found) - isc::dhcp::Subnet4Ptr selectSubnet4o6(const Pkt4Ptr& query) const; + isc::dhcp::Subnet4Ptr selectSubnet4o6(Pkt4Ptr& query); /// indicates if shutdown is in progress. Setting it to true will /// initiate server shutdown procedure. diff --git a/src/bin/dhcp4/tests/dhcp4_test_utils.cc b/src/bin/dhcp4/tests/dhcp4_test_utils.cc index 0734a99229..bd02d5f6fd 100644 --- a/src/bin/dhcp4/tests/dhcp4_test_utils.cc +++ b/src/bin/dhcp4/tests/dhcp4_test_utils.cc @@ -631,7 +631,7 @@ Dhcpv4SrvTest::configure(const std::string& config, NakedDhcpv4Srv& srv, } Dhcpv4Exchange -Dhcpv4SrvTest::createExchange(const Pkt4Ptr& query) { +Dhcpv4SrvTest::createExchange(Pkt4Ptr& query) { return (Dhcpv4Exchange(srv_.alloc_engine_, query, srv_.selectSubnet(query))); } diff --git a/src/bin/dhcp4/tests/dhcp4_test_utils.h b/src/bin/dhcp4/tests/dhcp4_test_utils.h index 48e571a447..dd4c1a795b 100644 --- a/src/bin/dhcp4/tests/dhcp4_test_utils.h +++ b/src/bin/dhcp4/tests/dhcp4_test_utils.h @@ -434,7 +434,7 @@ public: uint8_t pkt_type, const std::string& stat_name); /// @brief Create @c Dhcpv4Exchange from client's query. - Dhcpv4Exchange createExchange(const Pkt4Ptr& query); + Dhcpv4Exchange createExchange(Pkt4Ptr& query); /// @brief Performs 4-way exchange to obtain new lease. /// diff --git a/src/bin/dhcp4/tests/fqdn_unittest.cc b/src/bin/dhcp4/tests/fqdn_unittest.cc index 264ec433b7..2afa40955d 100644 --- a/src/bin/dhcp4/tests/fqdn_unittest.cc +++ b/src/bin/dhcp4/tests/fqdn_unittest.cc @@ -396,7 +396,7 @@ public: // Test that server generates the appropriate FQDN option in response to // client's FQDN option. - void testProcessFqdn(const Pkt4Ptr& query, const uint8_t exp_flags, + void testProcessFqdn(Pkt4Ptr& query, const uint8_t exp_flags, const std::string& exp_domain_name, Option4ClientFqdn::DomainNameType exp_domain_type = Option4ClientFqdn::FULL) { @@ -524,7 +524,7 @@ public: /// packet must contain the hostname option /// /// @return a pointer to the hostname option constructed by the server - OptionStringPtr processHostname(const Pkt4Ptr& query, + OptionStringPtr processHostname(Pkt4Ptr& query, bool must_have_host = true) { if (!getHostnameOption(query) && must_have_host) { ADD_FAILURE() << "Hostname option not carried in the query"; -- 2.47.2