[#1064] Unpack and set msgtype type in buffer4_received

src/hooks/dhcp/bootp/Makefile.am

@@ -30,6 +30,7 @@ libdhcp_bootp_la_SOURCES  =
 libdhcp_bootp_la_LDFLAGS  = $(AM_LDFLAGS)
 libdhcp_bootp_la_LDFLAGS  += -avoid-version -export-dynamic -module
 libdhcp_bootp_la_LIBADD = libbootp.la
+libdhcp_bootp_la_LIBADD += $(top_builddir)/src/lib/stats/libkea-stats.la
 libdhcp_bootp_la_LIBADD += $(top_builddir)/src/lib/dhcp/libkea-dhcp++.la
 libdhcp_bootp_la_LIBADD += $(top_builddir)/src/lib/hooks/libkea-hooks.la
 libdhcp_bootp_la_LIBADD += $(top_builddir)/src/lib/database/libkea-database.la

src/hooks/dhcp/bootp/bootp.dox

@@ -1,4 +1,4 @@
-// Copyright (C) 2019 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2019-2010 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -53,8 +53,8 @@ bootp_callouts.cc. It checks if the necessary parameter is passed and
 decodes the option configurations. @ref unload() free the configuration.
 
 Kea engine checks if the library has functions that match known hook
-point names. This library has two such functions: @ref buffer4_receive
+point names. This library has one such function: @ref buffer4_receive
-and @ref pkt4_receive located in bootp_callouts.cc.
+located in bootp_callouts.cc.
 
 If the receive query has to dhcp-message-type option then it is a BOOTP
 one: the BOOTP client class and a DHCPREQUEST dhcp-message-type option

src/hooks/dhcp/bootp/bootp_callouts.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2019 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2019-2020 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the End User License
 // Agreement. See COPYING file in the premium/ directory.
@@ -8,12 +8,14 @@
 #include <bootp_log.h>
 #include <hooks/hooks.h>
 #include <dhcp/pkt4.h>
+#include <stats/stats_mgr.h>
 
 using namespace isc;
 using namespace isc::bootp;
 using namespace isc::dhcp;
 using namespace isc::hooks;
 using namespace isc::log;
+using namespace isc::stats;
 
 // Functions accessed by the hooks framework use C linkage to avoid the name
 // mangling that accompanies use of the C++ compiler as well as to avoid
@@ -23,7 +25,8 @@ extern "C" {
 /// @brief This callout is called at the "buffer4_receive" hook.
 ///
 /// Ignore DHCP and BOOTREPLY messages.
-/// Remaining packets should be BOOTP requests so add the BOOTP client class,
+/// Remaining packets should be BOOTP requests so add the BOOTP client class
+/// and set the message type to DHCPREQUEST.
 ///
 /// @param handle CalloutHandle.
 ///
@@ -34,53 +37,47 @@ int buffer4_receive(CalloutHandle& handle) {
     handle.getArgument("query4", query);
 
     try {
-        // Take a copy and unpack it.
+        // Unpack it (TODO check if it was already unpacked).
-        Pkt4Ptr copy(new Pkt4(&query->data_[0], query->data_.size()));
+        query->unpack();
-        copy->unpack();
 
-        if (copy->getType() != DHCP_NOTYPE) {
+        // Not DHCP query nor BOOTP response?
-            // DHCP query.
+        if ((query->getType() == DHCP_NOTYPE) &&
-            return (0);
+            (query->getOp() == BOOTREQUEST)) {
+
+            query->addClass("BOOTP");
+            query->setType(DHCPREQUEST);
+
+            LOG_DEBUG(bootp_logger, DBGLVL_TRACE_BASIC, BOOTP_BOOTP_QUERY)
+                .arg(query->getLabel());
         }
-        if (copy->getOp() == BOOTREPLY) {
+    } catch (const SkipRemainingOptionsError& ex) {
-            // BOOTP response.
+        // An option failed to unpack but we are to attempt to process it
-            return (0);
+        // anyway.  Log it and let's hope for the best.
-        }
+        LOG_DEBUG(bootp_logger, DBGLVL_TRACE_BASIC,
-        // BOOTP query.
+                  BOOTP_PACKET_OPTIONS_SKIPPED)
-        query->addClass("BOOTP");
+            .arg(ex.what());
-        LOG_DEBUG(bootp_logger, DBGLVL_TRACE_BASIC, BOOTP_ADDED_CLASS)
+    } catch (const std::exception& ex) {
-            .arg(query->getLabel());
+        // Failed to parse the packet.
-    } catch (const std::exception&) {
+        LOG_DEBUG(bootp_logger, DBGLVL_TRACE_BASIC,
-        // Got an error. The query shall very likely be dropped later.
+                  BOOTP_PACKET_UNPACK_FAILED)
-        // Note this covers malformed DHCP packets where the message
+            .arg(query->getRemoteAddr().toText())
-        // type option can't be unpacked, and RFC 951 BOOTP.
+            .arg(query->getLocalAddr().toText())
+            .arg(query->getIface())
+            .arg(ex.what());
+
+        // Increase the statistics of parse failures and dropped packets.
+        StatsMgr::instance().addValue("pkt4-parse-failed",
+                                      static_cast<int64_t>(1));
+        StatsMgr::instance().addValue("pkt4-receive-drop",
+                                      static_cast<int64_t>(1));
+
+        handle.setStatus(CalloutHandle::NEXT_STEP_DROP);
+
+        return (0);
     }
 
-    return (0);
+    // Avoid to unpack it a second time!
-}
+    handle.setStatus(CalloutHandle::NEXT_STEP_SKIP);
-
-/// @brief This callout is called at the "pkt4_receive" hook.
-///
-/// If in the BOOTP class add a DHCPREQUEST dhcp-message-type.
-///
-/// @param handle CalloutHandle.
-///
-/// @return 0 upon success, non-zero otherwise.
-int pkt4_receive(CalloutHandle& handle) {
-    // Get the received unpacked message.
-    Pkt4Ptr query;
-    handle.getArgument("query4", query);
-
-    try {
-        if (!query->inClass("BOOTP")) {
-            return (0);
-        }
-        query->setType(DHCPREQUEST);
-        LOG_DEBUG(bootp_logger, DBGLVL_TRACE_BASIC, BOOTP_ADDED_MSGTYPE)
-            .arg(query->getLabel());
-    } catch (const std::exception&) {
-        // Got an error (should not happen).
-    }
 
     return (0);
 }

src/hooks/dhcp/bootp/bootp_messages.cc

@@ -1,20 +1,22 @@
-// File created from ../../../../src/hooks/dhcp/bootp/bootp_messages.mes on Fri Nov 22 2019 01:05
+// File created from ../../../../src/hooks/dhcp/bootp/bootp_messages.mes on Mon Jan 20 2020 17:15
 
 #include <cstddef>
 #include <log/message_types.h>
 #include <log/message_initializer.h>
 
-extern const isc::log::MessageID BOOTP_ADDED_CLASS = "BOOTP_ADDED_CLASS";
+extern const isc::log::MessageID BOOTP_BOOTP_QUERY = "BOOTP_BOOTP_QUERY";
-extern const isc::log::MessageID BOOTP_ADDED_MSGTYPE = "BOOTP_ADDED_MSGTYPE";
 extern const isc::log::MessageID BOOTP_LOAD = "BOOTP_LOAD";
+extern const isc::log::MessageID BOOTP_PACKET_OPTIONS_SKIPPED = "BOOTP_PACKET_OPTIONS_SKIPPED";
+extern const isc::log::MessageID BOOTP_PACKET_UNPACK_FAILED = "BOOTP_PACKET_UNPACK_FAILED";
 extern const isc::log::MessageID BOOTP_UNLOAD = "BOOTP_UNLOAD";
 
 namespace {
 
 const char* values[] = {
-    "BOOTP_ADDED_CLASS", "added BOOTP class to a BOOTP query: %1",
+    "BOOTP_BOOTP_QUERY", "recognized a BOOTP query: %1",
-    "BOOTP_ADDED_MSGTYPE", "added DHCPREQUEST message type to a BOOTP query: %1",
     "BOOTP_LOAD", "Bootp hooks library has been loaded",
+    "BOOTP_PACKET_OPTIONS_SKIPPED", "an error upacking an option, caused subsequent options to be skipped: %1",
+    "BOOTP_PACKET_UNPACK_FAILED", "failed to parse query from %1 to %2, received over interface %3, reason: %4",
     "BOOTP_UNLOAD", "Bootp hooks library has been unloaded",
     NULL
 };

src/hooks/dhcp/bootp/bootp_messages.h

@@ -1,13 +1,14 @@
-// File created from ../../../../src/hooks/dhcp/bootp/bootp_messages.mes on Fri Nov 22 2019 01:05
+// File created from ../../../../src/hooks/dhcp/bootp/bootp_messages.mes on Mon Jan 20 2020 17:15
 
 #ifndef BOOTP_MESSAGES_H
 #define BOOTP_MESSAGES_H
 
 #include <log/message_types.h>
 
-extern const isc::log::MessageID BOOTP_ADDED_CLASS;
+extern const isc::log::MessageID BOOTP_BOOTP_QUERY;
-extern const isc::log::MessageID BOOTP_ADDED_MSGTYPE;
 extern const isc::log::MessageID BOOTP_LOAD;
+extern const isc::log::MessageID BOOTP_PACKET_OPTIONS_SKIPPED;
+extern const isc::log::MessageID BOOTP_PACKET_UNPACK_FAILED;
 extern const isc::log::MessageID BOOTP_UNLOAD;
 
 #endif // BOOTP_MESSAGES_H

src/hooks/dhcp/bootp/bootp_messages.mes

@@ -1,17 +1,25 @@
-# Copyright (C) 2019 Internet Systems Consortium, Inc. ("ISC")
+# Copyright (C) 2019-2020 Internet Systems Consortium, Inc. ("ISC")
 
-% BOOTP_ADDED_CLASS added BOOTP class to a BOOTP query: %1
+% BOOTP_BOOTP_QUERY recognized a BOOTP query: %1
-This debug message is printed when the BOOTP client class was added to
+This debug message is printed when the BOOTP query was recognized. The
-a BOOTP query.  The query client and transaction identification are
+BOOTP client class was added and the message type set to DHCPREQUEST.
-displayed.
+The query client and transaction identification are displayed.
-
-% BOOTP_ADDED_MSGTYPE added DHCPREQUEST message type to a BOOTP query: %1
-This debug message is printed when the DHCPREQUEST message type was
-added to a BOOTP query.  The query client and transaction
-identification are displayed.
 
 % BOOTP_LOAD Bootp hooks library has been loaded
 This info message indicates that the Bootp hooks library has been loaded.
 
+% BOOTP_PACKET_OPTIONS_SKIPPED an error upacking an option, caused subsequent options to be skipped: %1
+A debug message issued when an option failed to unpack correctly, making it
+impossible to unpack the remaining options in the DHCPv4 query. The server
+will still attempt to service the packet. The sole argument provides a
+reason for unpacking error.
+
+% BOOTP_PACKET_UNPACK_FAILED failed to parse query from %1 to %2, received over interface %3, reason: %4
+This debug message is issued when received DHCPv4 query is malformed and
+can't be parsed by the buffer4_receive callout. The query will be
+dropped by the server. The first three arguments specify source IP address,
+destination IP address and the interface. The last argument provides a
+reason for failure.
+
 % BOOTP_UNLOAD Bootp hooks library has been unloaded
 This info message indicates that the Bootp hooks library has been unloaded.

src/hooks/dhcp/bootp/tests/Makefile.am

@@ -35,6 +35,7 @@ bootp_unittests_LDFLAGS  = $(AM_LDFLAGS) $(CRYPTO_LDFLAGS) $(GTEST_LDFLAGS)
 bootp_unittests_CXXFLAGS = $(AM_CXXFLAGS)
 
 bootp_unittests_LDADD  = $(top_builddir)/src/hooks/dhcp/bootp/libbootp.la
+bootp_unittests_LDADD += $(top_builddir)/src/lib/stats/libkea-stats.la
 bootp_unittests_LDADD += $(top_builddir)/src/lib/dhcp/libkea-dhcp++.la
 bootp_unittests_LDADD += $(top_builddir)/src/lib/hooks/libkea-hooks.la
 bootp_unittests_LDADD += $(top_builddir)/src/lib/database/libkea-database.la

src/hooks/dhcp/bootp/tests/bootp_unittests.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2019 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2019-2020 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -24,7 +24,6 @@ using namespace isc::util;
 
 extern "C" {
 extern int buffer4_receive(CalloutHandle& handle);
-extern int pkt4_receive(CalloutHandle& handle);
 }
 
 namespace {
@@ -48,7 +47,7 @@ public:
         return(co_manager_);
     }
 
-    /// @brief Tests buffer4_receive and pkt4_receive callout.
+    /// @brief Tests buffer4_receive callout.
     ///
     /// @param pkt The packet to submit.
     /// @param processed True if the packet must be processed, false otherwise.
@@ -56,38 +55,32 @@ public:
         // Get callout handle.
         CalloutHandle handle(getCalloutManager());
 
-        // Fill data so it becomes possible to unpack a copy of it.
+        // Get type.
+        uint8_t type = pkt->getType();
+
+        // Get data so it becomes possible to reset it to unpacked state.
         ASSERT_NO_THROW(pkt->pack());
         const OutputBuffer& buffer = pkt->getBuffer();
-        pkt->data_.resize(buffer.getLength());
+        pkt.reset(new Pkt4(reinterpret_cast<const uint8_t*>(buffer.getData()),
-        memmove(&pkt->data_[0], buffer.getData(), pkt->data_.size());
+                           buffer.getLength()));
 
         // Set query.
         handle.setArgument("query4", pkt);
 
-        // Get type.
-        uint8_t type = pkt->getType();
-
         // Execute buffer4_receive callout.
         int ret;
         ASSERT_NO_THROW(ret = buffer4_receive(handle));
         EXPECT_EQ(0, ret);
 
+        // Verify status (SKIP means unpacked).
+        EXPECT_EQ(CalloutHandle::NEXT_STEP_SKIP, handle.getStatus());
+
         // Verify processing.
         if (processed) {
             EXPECT_TRUE(pkt->inClass("BOOTP"));
-        } else {
-            EXPECT_FALSE(pkt->inClass("BOOTP"));
-        }
-
-        // Execute pkt4_receive callout.
-        ASSERT_NO_THROW(ret = pkt4_receive(handle));
-        EXPECT_EQ(0, ret);
-
-        // Verify processing.
-        if (processed) {
             EXPECT_EQ(DHCPREQUEST, pkt->getType());
         } else {
+            EXPECT_FALSE(pkt->inClass("BOOTP"));
             EXPECT_EQ(type, pkt->getType());
         }
     }