From: Francis Dupont Date: Mon, 25 Sep 2017 13:27:54 +0000 (+0200) Subject: [5073a] Addressed review comments X-Git-Tag: trac5363_base~6^2~2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=5615b95f585e3dc935d94fb9c689aa6ef5dc187d;p=thirdparty%2Fkea.git [5073a] Addressed review comments --- diff --git a/doc/examples/kea4/classify.json b/doc/examples/kea4/classify.json index fbe4753ec0..6964960e22 100644 --- a/doc/examples/kea4/classify.json +++ b/doc/examples/kea4/classify.json @@ -55,7 +55,7 @@ // In this particular class, we want to set specific values // of certain DHCPv4 fields. If the incoming packet matches the // test, those fields will be set in outgoing responses. -// The option 43 is defined to encapsulate suboption inf the aastra space. +// The option 43 is defined to encapsulate suboption in the aastra space. { "name": "VoIP", "test": "substring(option[60].hex,0,6) == 'Aastra'", diff --git a/doc/guide/dhcp4-srv.xml b/doc/guide/dhcp4-srv.xml index 4574e622e9..6b1ec5928c 100644 --- a/doc/guide/dhcp4-srv.xml +++ b/doc/guide/dhcp4-srv.xml @@ -1528,9 +1528,9 @@ It is merely echoed by the server They can be defined at the global scope or at client class local scope: this allows to use option definitions depending on context and to set option data accordingly. - As the Vendor Specific Information option (code 43) can carry - in a not compatible way a raw binary value or sub-options this - mechanism is available for this option too. + As the Vendor Specific Information option (code 43) has vendor + specific format, i.e. can carry either raw binary value or + sub-options, this mechanism is available for this option too. The definition used to decode a VSI option is: @@ -1637,6 +1637,11 @@ It is merely echoed by the server ... } + + + Another possibility adde din Kea 1.3 is to redefine the option, + see . +
diff --git a/src/bin/dhcp4/dhcp4.dox b/src/bin/dhcp4/dhcp4.dox index 190fa56140..e6034b5c54 100644 --- a/src/bin/dhcp4/dhcp4.dox +++ b/src/bin/dhcp4/dhcp4.dox @@ -171,7 +171,7 @@ Private (codes 224-254) and VSI (code 43) options are not decoded by @c LibDHCP::unpackOptions4 but by @ref isc::dhcp::Dhcpv4Srv::deferredUnpack function after classification. To make this function to perform or not deferred processing the simplest is to add or not the option code -to the @ref isc::dhcp::Pkt4::deferredOptions list. +to the @ref isc::dhcp::Pkt4::getDeferredOptions list. @section dhcpv4DDNSIntegration DHCPv4 Server Support for the Dynamic DNS Updates The DHCPv4 server supports processing of the DHCPv4 Client FQDN option (RFC4702) diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index c968a7d3bc..521e7f267c 100644 --- a/src/bin/dhcp4/dhcp4_srv.cc +++ b/src/bin/dhcp4/dhcp4_srv.cc @@ -2827,7 +2827,7 @@ void Dhcpv4Srv::deferredUnpack(Pkt4Ptr& query) { // Iterate on the list of deferred option codes - BOOST_FOREACH(const uint16_t& code, query->deferredOptions()) { + BOOST_FOREACH(const uint16_t& code, query->getDeferredOptions()) { OptionDefinitionPtr def; // Iterate on client classes const ClientClasses& classes = query->getClasses(); @@ -2869,12 +2869,12 @@ Dhcpv4Srv::deferredUnpack(Pkt4Ptr& query) // Get the existing option for its content and remove all OptionPtr opt = query->getOption(code); if (!opt) { - /* should not happen but do not crash anyway */ + // should not happen but do not crash anyway continue; } const OptionBuffer buf = opt->getData(); while (query->delOption(code)) { - /* continue */ + // continue } // Unpack the option and add it opt = def->optionFactory(Option::V4, code, buf.cbegin(), buf.cend()); diff --git a/src/bin/dhcp4/dhcp4_srv.h b/src/bin/dhcp4/dhcp4_srv.h index 38053c5543..3d16aa9e87 100644 --- a/src/bin/dhcp4/dhcp4_srv.h +++ b/src/bin/dhcp4/dhcp4_srv.h @@ -808,7 +808,10 @@ protected: /// @note Options 43 and 224-254 are processed after classification. /// If a class configures a definition it is applied, if none /// the global (user) definition is applied. For option 43 - /// a last resort definition (same than for previous Kea) is used. + /// a last resort definition (same definition as used in previous Kea + /// versions) is applied when none is found. + /// + /// @param query Pointer to the client message. void deferredUnpack(Pkt4Ptr& query); /// @brief Allocation Engine. diff --git a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc index ec1a35585e..85897604b1 100644 --- a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc +++ b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc @@ -2300,7 +2300,9 @@ TEST_F(Dhcpv4SrvTest, option43BadRaw) { NakedDhcpv4Srv srv(0); // The vendor-encapsulated-options has an incompatible data - // so won't have the expected content. + // so won't have the expected content but processing of truncated + // (suboption length > available length) suboptions does not raise + // an exception. string config = "{ \"interfaces-config\": {" " \"interfaces\": [ \"*\" ] }, " "\"rebind-timer\": 2000, " @@ -2343,7 +2345,7 @@ TEST_F(Dhcpv4SrvTest, option43BadRaw) { buf.push_back(0x02); OptionPtr vopt(new Option(Option::V4, DHO_VENDOR_ENCAPSULATED_OPTIONS, buf)); query->addOption(vopt); - query->deferredOptions().push_back(DHO_VENDOR_ENCAPSULATED_OPTIONS); + query->getDeferredOptions().push_back(DHO_VENDOR_ENCAPSULATED_OPTIONS); // Create and add a PRL option to the query OptionUint8ArrayPtr prl(new OptionUint8Array(Option::V4, @@ -2386,7 +2388,9 @@ TEST_F(Dhcpv4SrvTest, option43FailRaw) { NakedDhcpv4Srv srv(0); // The vendor-encapsulated-options has an incompatible data - // so won't have the expected content. + // so won't have the expected content. Here the processing + // of suboptions tries to unpack the uitn32 foo suboption and + // raises an exception. string config = "{ \"interfaces-config\": {" " \"interfaces\": [ \"*\" ] }, " "\"rebind-timer\": 2000, " @@ -2436,7 +2440,7 @@ TEST_F(Dhcpv4SrvTest, option43FailRaw) { buf.push_back(0x01); OptionPtr vopt(new Option(Option::V4, DHO_VENDOR_ENCAPSULATED_OPTIONS, buf)); query->addOption(vopt); - query->deferredOptions().push_back(DHO_VENDOR_ENCAPSULATED_OPTIONS); + query->getDeferredOptions().push_back(DHO_VENDOR_ENCAPSULATED_OPTIONS); // Create and add a PRL option to the query OptionUint8ArrayPtr prl(new OptionUint8Array(Option::V4, @@ -2505,7 +2509,7 @@ TEST_F(Dhcpv4SrvTest, option43RawGlobal) { buf.push_back(0x03); OptionPtr vopt(new Option(Option::V4, DHO_VENDOR_ENCAPSULATED_OPTIONS, buf)); query->addOption(vopt); - query->deferredOptions().push_back(DHO_VENDOR_ENCAPSULATED_OPTIONS); + query->getDeferredOptions().push_back(DHO_VENDOR_ENCAPSULATED_OPTIONS); // Create and add a PRL option to the query OptionUint8ArrayPtr prl(new OptionUint8Array(Option::V4, @@ -2600,7 +2604,7 @@ TEST_F(Dhcpv4SrvTest, option43RawClass) { buf.push_back(0x03); OptionPtr vopt(new Option(Option::V4, DHO_VENDOR_ENCAPSULATED_OPTIONS, buf)); query->addOption(vopt); - query->deferredOptions().push_back(DHO_VENDOR_ENCAPSULATED_OPTIONS); + query->getDeferredOptions().push_back(DHO_VENDOR_ENCAPSULATED_OPTIONS); // Create and add a PRL option to the query OptionUint8ArrayPtr prl(new OptionUint8Array(Option::V4, @@ -2705,7 +2709,7 @@ TEST_F(Dhcpv4SrvTest, option43Class) { buf.push_back(0x21); OptionPtr vopt(new Option(Option::V4, DHO_VENDOR_ENCAPSULATED_OPTIONS, buf)); query->addOption(vopt); - query->deferredOptions().push_back(DHO_VENDOR_ENCAPSULATED_OPTIONS); + query->getDeferredOptions().push_back(DHO_VENDOR_ENCAPSULATED_OPTIONS); // Create and add a vendor-class-identifier (code 60) OptionStringPtr iopt(new OptionString(Option::V4, @@ -2839,7 +2843,7 @@ TEST_F(Dhcpv4SrvTest, option43ClassPriority) { buf.push_back(0x21); OptionPtr vopt(new Option(Option::V4, DHO_VENDOR_ENCAPSULATED_OPTIONS, buf)); query->addOption(vopt); - query->deferredOptions().push_back(DHO_VENDOR_ENCAPSULATED_OPTIONS); + query->getDeferredOptions().push_back(DHO_VENDOR_ENCAPSULATED_OPTIONS); // Create and add a vendor-class-identifier (code 60) OptionStringPtr iopt(new OptionString(Option::V4, @@ -2979,7 +2983,7 @@ TEST_F(Dhcpv4SrvTest, option43Classes) { buf.push_back(0x21); OptionPtr vopt(new Option(Option::V4, DHO_VENDOR_ENCAPSULATED_OPTIONS, buf)); query->addOption(vopt); - query->deferredOptions().push_back(DHO_VENDOR_ENCAPSULATED_OPTIONS); + query->getDeferredOptions().push_back(DHO_VENDOR_ENCAPSULATED_OPTIONS); // Create and add a vendor-class-identifier (code 60) OptionStringPtr iopt(new OptionString(Option::V4, @@ -3086,7 +3090,7 @@ TEST_F(Dhcpv4SrvTest, privateOption) { buf.push_back(0x01); OptionPtr opt1(new Option(Option::V4, 234, buf)); query->addOption(opt1); - query->deferredOptions().push_back(234); + query->getDeferredOptions().push_back(234); // Create and add a private option with code 245 buf.clear(); @@ -3096,7 +3100,7 @@ TEST_F(Dhcpv4SrvTest, privateOption) { buf.push_back(0x21); OptionPtr opt2(new Option(Option::V4, 245, buf)); query->addOption(opt2); - query->deferredOptions().push_back(245); + query->getDeferredOptions().push_back(245); // Create and add a PRL option to the query diff --git a/src/lib/dhcp/libdhcp++.cc b/src/lib/dhcp/libdhcp++.cc index ae430fa487..bc9fba18db 100644 --- a/src/lib/dhcp/libdhcp++.cc +++ b/src/lib/dhcp/libdhcp++.cc @@ -294,7 +294,7 @@ LibDHCP::getLastResortOptionDefs(const std::string& space) { } bool -LibDHCP::deferOption(const std::string& space, const uint16_t code) { +LibDHCP::shouldDeferOptionUnpack(const std::string& space, const uint16_t code) { return ((space == DHCP4_OPTION_SPACE) && ((code == DHO_VENDOR_ENCAPSULATED_OPTIONS) || ((code >= 224) && (code <= 254)))); @@ -546,7 +546,7 @@ size_t LibDHCP::unpackOptions4(const OptionBuffer& buf, } // Check if option unpacking must be deferred - if (deferOption(option_space, opt_type)) { + if (shouldDeferOptionUnpack(option_space, opt_type)) { num_defs = 0; deferred.push_back(opt_type); } diff --git a/src/lib/dhcp/libdhcp++.h b/src/lib/dhcp/libdhcp++.h index b3f8d7f452..c505656793 100644 --- a/src/lib/dhcp/libdhcp++.h +++ b/src/lib/dhcp/libdhcp++.h @@ -144,11 +144,12 @@ public: /// /// DHCPv4 option 43 and 224-254 unpacking is done after classification. /// - /// @space Option space name. + /// @param space Option space name. /// @param code Option code. /// /// @return True if option processing should be deferred. - static bool deferOption(const std::string& space, const uint16_t code); + static bool shouldDeferOptionUnpack(const std::string& space, + const uint16_t code); /// @brief Factory function to create instance of option. /// @@ -381,10 +382,13 @@ private: /// is incorrect. This is a programming error. static void initStdOptionDefs6(); + /// Initialize last resort DHCPv4 option definitions. static void initLastResortOptionDefs(); + /// Initialize DOCSIS DHCPv4 option definitions. static void initVendorOptsDocsis4(); + /// Initialize DOCSIS DHCPv6 option definitions. static void initVendorOptsDocsis6(); /// Initialize private DHCPv6 option definitions. diff --git a/src/lib/dhcp/pkt4.h b/src/lib/dhcp/pkt4.h index 3c2607fa01..cd31fadb80 100644 --- a/src/lib/dhcp/pkt4.h +++ b/src/lib/dhcp/pkt4.h @@ -364,8 +364,14 @@ public: return (local_hwaddr_); } - /// @brief Returns a reference to deferred option codes - std::list& deferredOptions() { + /// @brief Returns a reference to option codes which unpacking + /// will be deferred. + /// + /// @notes Only options 42 and 224-254 are subject of deferred + /// unpacking: when the packet unpacking is performed each time + /// such an option is found it is unpacked as an unknown option + /// and the code added in this list. + std::list& getDeferredOptions() { return (deferred_options_); } diff --git a/src/lib/dhcp/std_option_defs.h b/src/lib/dhcp/std_option_defs.h index 04f8488ec3..16248e0882 100644 --- a/src/lib/dhcp/std_option_defs.h +++ b/src/lib/dhcp/std_option_defs.h @@ -216,7 +216,8 @@ const OptionDefParams STANDARD_V4_OPTION_DEFINITIONS[] = { const int STANDARD_V4_OPTION_DEFINITIONS_SIZE = sizeof(STANDARD_V4_OPTION_DEFINITIONS) / sizeof(STANDARD_V4_OPTION_DEFINITIONS[0]); -/// Last resort definitions (only option 43 for now). +/// Last resort definitions (only option 43 for now, these definitions +/// are applied in deferred unpacking when none is found). const OptionDefParams LAST_RESORT_V4_OPTION_DEFINITIONS[] = { { "vendor-encapsulated-options", DHO_VENDOR_ENCAPSULATED_OPTIONS, OPT_EMPTY_TYPE, false, NO_RECORD_DEF, "vendor-encapsulated-options-space" } diff --git a/src/lib/dhcp/tests/libdhcp++_unittest.cc b/src/lib/dhcp/tests/libdhcp++_unittest.cc index 534a1fd967..7a09681d69 100644 --- a/src/lib/dhcp/tests/libdhcp++_unittest.cc +++ b/src/lib/dhcp/tests/libdhcp++_unittest.cc @@ -1815,10 +1815,10 @@ TEST_F(LibDhcpTest, setRuntimeOptionDefs) { // This test verifies the processing of option 43 TEST_F(LibDhcpTest, option43) { - // Check deferOption() - EXPECT_TRUE(LibDHCP::deferOption(DHCP4_OPTION_SPACE, 43)); - EXPECT_FALSE(LibDHCP::deferOption(DHCP4_OPTION_SPACE, 44)); - EXPECT_FALSE(LibDHCP::deferOption(DHCP6_OPTION_SPACE, 43)); + // Check shouldDeferOptionUnpack() + EXPECT_TRUE(LibDHCP::shouldDeferOptionUnpack(DHCP4_OPTION_SPACE, 43)); + EXPECT_FALSE(LibDHCP::shouldDeferOptionUnpack(DHCP4_OPTION_SPACE, 44)); + EXPECT_FALSE(LibDHCP::shouldDeferOptionUnpack(DHCP6_OPTION_SPACE, 43)); // Check last resort OptionDefinitionPtr def; diff --git a/src/lib/dhcpsrv/parsers/client_class_def_parser.cc b/src/lib/dhcpsrv/parsers/client_class_def_parser.cc index a630b47c90..9d6eba7613 100644 --- a/src/lib/dhcpsrv/parsers/client_class_def_parser.cc +++ b/src/lib/dhcpsrv/parsers/client_class_def_parser.cc @@ -101,7 +101,8 @@ ClientClassDefParser::parse(ClientClassDictionaryPtr& class_dictionary, def = parser.parse(option_def); // Verify if the defition is for an option which are // in a deferred processing list. - if (!LibDHCP::deferOption(def.second, def.first->getCode())) { + if (!LibDHCP::shouldDeferOptionUnpack(def.second, + def.first->getCode())) { isc_throw(DhcpConfigError, "Not allowed option definition for code '" << def.first->getCode() << "' in space '"