From: Francis Dupont Date: Sun, 22 Nov 2020 11:51:51 +0000 (+0100) Subject: [#1449] Fixed v6 appending multiple option instances X-Git-Tag: Kea-1.9.3~146 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=7c5455e5a63bd797c1d5c1b63fb8d348d09b1bf8;p=thirdparty%2Fkea.git [#1449] Fixed v6 appending multiple option instances --- diff --git a/ChangeLog b/ChangeLog index 90fb429bf5..aa04767d08 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,9 @@ +1838. [bug] fdupont + The DHCPv6 sent multiple instances of an option or a + sub-option when it seems to be requested more than once + directly by the client or using the always-send flag. + (Gitlab #1449) + 1837. [doc] cstrotm Several Kea ARM corrections. (Gitlab #1514) diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index ba9f90a366..8280455606 100644 --- a/src/bin/dhcp4/dhcp4_srv.cc +++ b/src/bin/dhcp4/dhcp4_srv.cc @@ -1859,7 +1859,7 @@ Dhcpv4Srv::appendRequestedVendorOptions(Dhcpv4Exchange& ex) { bool added = false; for (std::vector::const_iterator code = requested_opts.begin(); code != requested_opts.end(); ++code) { - if (!vendor_rsp->getOption(*code)) { + if (!vendor_rsp->getOption(*code)) { for (CfgOptionList::const_iterator copts = co_list.begin(); copts != co_list.end(); ++copts) { OptionDescriptor desc = (*copts)->get(vendor_id, *code); diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc index 7544a52fac..b723bb9ab4 100644 --- a/src/bin/dhcp6/dhcp6_srv.cc +++ b/src/bin/dhcp6/dhcp6_srv.cc @@ -1384,15 +1384,18 @@ Dhcpv6Srv::appendRequestedOptions(const Pkt6Ptr& question, Pkt6Ptr& answer, } } - BOOST_FOREACH(uint16_t opt, requested_opts) { - // Iterate on the configured option list - for (CfgOptionList::const_iterator copts = co_list.begin(); - copts != co_list.end(); ++copts) { - OptionDescriptor desc = (*copts)->get(DHCP6_OPTION_SPACE, opt); - // Got it: add it and jump to the outer loop - if (desc.option_) { - answer->addOption(desc.option_); - break; + for (uint16_t opt : requested_opts) { + // Add nothing when it is already there. + if (!answer->getOption(opt)) { + // Iterate on the configured option list + for (CfgOptionList::const_iterator copts = co_list.begin(); + copts != co_list.end(); ++copts) { + OptionDescriptor desc = (*copts)->get(DHCP6_OPTION_SPACE, opt); + // Got it: add it and jump to the outer loop + if (desc.option_) { + answer->addOption(desc.option_); + break; + } } } } @@ -1488,14 +1491,16 @@ Dhcpv6Srv::appendRequestedVendorOptions(const Pkt6Ptr& question, // Get the list of options that client requested. bool added = false; - BOOST_FOREACH(uint16_t opt, requested_opts) { + for (uint16_t opt : requested_opts) { for (CfgOptionList::const_iterator copts = co_list.begin(); copts != co_list.end(); ++copts) { - OptionDescriptor desc = (*copts)->get(vendor_id, opt); - if (desc.option_) { - vendor_rsp->addOption(desc.option_); - added = true; - break; + if (!vendor_rsp->getOption(opt)) { + OptionDescriptor desc = (*copts)->get(vendor_id, opt); + if (desc.option_) { + vendor_rsp->addOption(desc.option_); + added = true; + break; + } } } } diff --git a/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc b/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc index 8108b8c225..192c7aed81 100644 --- a/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc +++ b/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc @@ -2221,6 +2221,28 @@ TEST_F(Dhcpv6SrvTest, prlPersistency) { ASSERT_TRUE(response->getOption(D6O_NAME_SERVERS)); // and still no sntp-servers ASSERT_FALSE(response->getOption(D6O_SNTP_SERVERS)); + + // Reset ORO adding subscriber-id + sol->delOption(D6O_ORO); + OptionUint16ArrayPtr oro2(new OptionUint16Array(Option::V6, D6O_ORO)); + ASSERT_TRUE(oro2); + oro2->addValue(D6O_SUBSCRIBER_ID); + sol->addOption(oro2); + + // Let the server process it again. + AllocEngine::ClientContext6 ctx3; + srv_.initContext(sol, ctx3, drop); + ASSERT_FALSE(drop); + response = srv_.processSolicit(ctx3); + + // The subscriber-id option should be present but only once despite + // it is both requested and has always-send. + const OptionCollection& sifs = response->getOptions(D6O_SUBSCRIBER_ID); + ASSERT_EQ(1, sifs.size()); + // But no dns-servers + ASSERT_FALSE(response->getOption(D6O_NAME_SERVERS)); + // Nor a sntp-servers + ASSERT_FALSE(response->getOption(D6O_SNTP_SERVERS)); } // Checks if server is able to handle a relayed traffic from DOCSIS3.0 modems diff --git a/src/bin/dhcp6/tests/vendor_opts_unittest.cc b/src/bin/dhcp6/tests/vendor_opts_unittest.cc index 0836d0101d..e869c69c24 100644 --- a/src/bin/dhcp6/tests/vendor_opts_unittest.cc +++ b/src/bin/dhcp6/tests/vendor_opts_unittest.cc @@ -268,6 +268,36 @@ TEST_F(VendorOptsTest, vendorPersistentOptions) { OptionStringPtr config_file = boost::dynamic_pointer_cast(docsis33); ASSERT_TRUE(config_file); EXPECT_EQ("normal_erouter_v6.cm", config_file->getValue()); + + // Let's add a vendor-option (vendor-id=4491) with a single sub-option. + // That suboption has code 1 and is a docsis ORO option. + sol->delOption(D6O_VENDOR_OPTS); + boost::shared_ptr vendor_oro(new OptionUint16Array(Option::V6, + DOCSIS3_V6_ORO)); + vendor_oro->addValue(DOCSIS3_V6_CONFIG_FILE); // Request option 33 + OptionPtr vendor2(new OptionVendor(Option::V6, 4491)); + vendor2->addOption(vendor_oro); + sol->addOption(vendor2); + + // Need to process SOLICIT again after requesting new option. + AllocEngine::ClientContext6 ctx2; + srv_.initContext(sol, ctx2, drop); + ASSERT_FALSE(drop); + adv = srv_.processSolicit(ctx2); + ASSERT_TRUE(adv); + + // Check if there is vendor option response + tmp = adv->getOption(D6O_VENDOR_OPTS); + ASSERT_TRUE(tmp); + + // The response should be OptionVendor object + vendor_resp = boost::dynamic_pointer_cast(tmp); + ASSERT_TRUE(vendor_resp); + + // There should be only one suboption despite config-file is both + // requested and has the always-send flag. + const OptionCollection& opts = vendor_resp->getOptions(); + ASSERT_EQ(1, opts.size()); } // Test checks whether it is possible to use option definitions defined in