From: Marcin Siodelski Date: Thu, 24 Jul 2014 11:19:04 +0000 (+0200) Subject: [3269] Address review comments. X-Git-Tag: trac3482_base~71^2~1 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=5e5f9047f4e529bb720f78d1682586c0e494b519;p=thirdparty%2Fkea.git [3269] Address review comments. --- diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc index 52dffb87be..c0566466ca 100644 --- a/src/bin/dhcp6/dhcp6_srv.cc +++ b/src/bin/dhcp6/dhcp6_srv.cc @@ -2326,9 +2326,10 @@ Dhcpv6Srv::processConfirm(const Pkt6Ptr& confirm) { // Make sure that the necessary options are included. copyDefaultOptions(confirm, reply); appendDefaultOptions(confirm, reply); - // Number of addresses verified. If at the end it occurs that no addresses - // were verified we will need to dicard the message. - int verified = 0; + // Indicates if at least one address has been verified. If no addresses + // are verified it means that the client has sent no IA_NA options + // or no IAAddr options and that client's message has to be discarded. + bool verified = false; // Check if subnet can be selected for the message. If no subnet // has been selected, the client is not on link. SubnetPtr subnet = selectSubnet(confirm); @@ -2342,28 +2343,29 @@ Dhcpv6Srv::processConfirm(const Pkt6Ptr& confirm) { for (OptionCollection::const_iterator opt = opts.begin(); opt != opts.end(); ++opt) { // Ignore options other than IAAddr. - if (opt->second->getType() != D6O_IAADDR) { - continue; - } - // Check that the address is in range in the subnet selected. - Option6IAAddrPtr iaaddr = boost::dynamic_pointer_cast< - Option6IAAddr>(opt->second); - // If there is subnet selected and the address has been included - // in IA_NA, mark it verified and verify that it belongs to the - // subnet. - if (iaaddr && subnet) { - // There is an address so we increase the counter of - // addresses verified. - ++verified; - // If at least one address is not in range, then return - // the NotOnLink status code. - if (!subnet->inRange(iaaddr->getAddress())) { - std::ostringstream status_msg; + if (opt->second->getType() == D6O_IAADDR) { + // Check that the address is in range in the subnet selected. + Option6IAAddrPtr iaaddr = boost::dynamic_pointer_cast< + Option6IAAddr>(opt->second); + // If there is subnet selected and the address has been included + // in IA_NA, mark it verified and verify that it belongs to the + // subnet. + if (iaaddr) { + verified = true; + // If at least one address is not in range, then return + // the NotOnLink status code. + if (subnet && !subnet->inRange(iaaddr->getAddress())) { + std::ostringstream status_msg; status_msg << "Address " << iaaddr->getAddress() << " is not on link."; reply->addOption(createStatusCode(STATUS_NotOnLink, status_msg.str())); return (reply); + } + } else { + isc_throw(Unexpected, "failed to cast the IA Address option" + " to the Option6IAAddrPtr. This is programmatic" + " error and should be reported"); } } } @@ -2371,7 +2373,7 @@ Dhcpv6Srv::processConfirm(const Pkt6Ptr& confirm) { // It seems that the client hasn't included any addresses in which case // the Confirm must be discarded. - if (verified == 0) { + if (!verified) { return (Pkt6Ptr()); } diff --git a/src/bin/dhcp6/tests/confirm_unittest.cc b/src/bin/dhcp6/tests/confirm_unittest.cc index 9a80631083..64d66cb0a8 100644 --- a/src/bin/dhcp6/tests/confirm_unittest.cc +++ b/src/bin/dhcp6/tests/confirm_unittest.cc @@ -32,11 +32,11 @@ namespace { /// /// - Configuration 0: /// - only addresses (no prefixes) -/// - 2 subnets with 2001:db8:1::/64 and 2001:db8:2::64 +/// - 2 subnets with 2001:db8:1::/64 and 2001:db8:2::/64 /// - 1 subnet for eth0 and 1 subnet for eth1 /// /// - Configuration 1: -/// - similar to Configuration 0 and Configuration 1 +/// - similar to Configuration 0 /// - pools configured: 3000:1::/64 and 3000:2::/64 /// - this specific configuration is used by tests using relays /// @@ -97,7 +97,7 @@ public: // Test that directly connected client's Confirm message is processed and Reply // message is sent back. In this test case, the client sends Confirm for two // addresses that belong to the same IAID and are sent within the same IA_NA -// option. +// option (RFC3315, section 18.2.2). TEST_F(ConfirmTest, directClientSameIAID) { Dhcp6Client client; // Configure client to request IA_NA. @@ -139,7 +139,7 @@ TEST_F(ConfirmTest, directClientSameIAID) { // Test that directly connected client's Confirm message is processed and Reply // message is sent back. In this test case, the client sends Confirm for two // addresses that belong to different IAIDs and are sent within the different -// IA_NA options. +// IA_NA options (RFC3315, section 18.2.2). TEST_F(ConfirmTest, directClientDifferentIAID) { Dhcp6Client client; // Configure client to request IA_NA. @@ -185,7 +185,7 @@ TEST_F(ConfirmTest, directClientDifferentIAID) { // Test that relayed client's Confirm message is processed and Reply message -// is sent back. +// is sent back (RFC3315, section 18.2.2). TEST_F(ConfirmTest, relayedClient) { Dhcp6Client client; // Client to send relayed message. @@ -226,7 +226,8 @@ TEST_F(ConfirmTest, relayedClient) { EXPECT_TRUE(client.getContext().response_->getOption(D6O_CLIENTID)); } -// Test that the Confirm message without any addresses is discarded. +// Test that the Confirm message without any addresses is discarded +// (RFC3315, section 18.2.2). TEST_F(ConfirmTest, relayedClientNoAddress) { Dhcp6Client client; // Configure the server. @@ -242,8 +243,48 @@ TEST_F(ConfirmTest, relayedClientNoAddress) { EXPECT_FALSE(client.getContext().response_); } +// This test checks that the server processes Confirm message correctly if +// the subnet can't be selected for the client (RFC3315, section 18.2.2). +TEST_F(ConfirmTest, relayedClientNoSubnet) { + Dhcp6Client client; + // Client to send relayed message. + client.useRelay(); + // Configure client to request IA_NA. + client.useNA(); + // Make 4-way exchange to get the lease. + ASSERT_NO_FATAL_FAILURE(requestLease(CONFIRM_CONFIGS[1], 2, client)); + // Now that the client has a lease, let's remove any subnets to check + // how the server would respond to the Confirm. + ASSERT_NO_THROW(CfgMgr::instance().deleteSubnets6()); + // Send Confirm message to the server. + ASSERT_NO_THROW(client.doConfirm()); + // Client should have received a status code option and this option should + // indicate that the client is NotOnLink becuase subnet could not be + // selected. + ASSERT_TRUE(client.receivedStatusCode()); + ASSERT_EQ(STATUS_NotOnLink, client.getStatusCode()); + + // Let's test another case that the client sends no addresses in the Confirm + // message. The subnet can't be selected for that client as in the previous + // case but this time the server must discard the client's message because + // it contains no addresses (is invalid). + + // Set lifetimes to 0 so as the Confirm will ignore the specific address + // and send an empty IA_NA. + client.config_.leases_[0].lease_.preferred_lft_ = 0; + client.config_.leases_[0].lease_.valid_lft_ = 0; + ASSERT_NO_THROW(client.doConfirm()); + EXPECT_FALSE(client.getContext().response_); + + // Do similar test but this time remove the lease so as no IA_NA option + // is sent. + client.config_.clear(); + ASSERT_NO_THROW(client.doConfirm()); + EXPECT_FALSE(client.getContext().response_); +} + // This test checks that the relayed Confirm messsage is processed by the server -// when sent to unicast address. +// when sent to unicast address RFC3315, section 18.2.8). TEST_F(ConfirmTest, relayedUnicast) { Dhcp6Client client; // Client to send relayed message. @@ -256,7 +297,7 @@ TEST_F(ConfirmTest, relayedUnicast) { ASSERT_GT(client.getLeaseNum(), 0); client.setDestAddress(IOAddress("2001:db8:1::1")); // Send Confirm message to the server. - ASSERT_NO_THROW(client.doConfirm()); + ASSERT_NO_THROW (client.doConfirm()); // Client should have received a response. ASSERT_TRUE(client.getContext().response_); // Client should have received a status code option and this option should diff --git a/src/bin/dhcp6/tests/dhcp6_client.cc b/src/bin/dhcp6/tests/dhcp6_client.cc index bcf4095385..da836ce9d6 100644 --- a/src/bin/dhcp6/tests/dhcp6_client.cc +++ b/src/bin/dhcp6/tests/dhcp6_client.cc @@ -178,7 +178,6 @@ Dhcp6Client::applyLease(const LeaseInfo& lease_info) { config_.leases_.push_back(lease_info); } - void Dhcp6Client::copyIAs(const Pkt6Ptr& source, const Pkt6Ptr& dest) { typedef OptionCollection Opts; @@ -208,18 +207,21 @@ Dhcp6Client::copyIAsFromLeases(const Pkt6Ptr& dest) const { opt->setT2(leases[0].t2_); for (std::vector::const_iterator lease = leases.begin(); lease != leases.end(); ++lease) { - if (lease->type_ == Lease::TYPE_NA) { - opt->addOption(Option6IAAddrPtr(new Option6IAAddr(D6O_IAADDR, + if ((lease->preferred_lft_ != 0) && (lease->valid_lft_ != 0)) { + if (lease->type_ == Lease::TYPE_NA) { + opt->addOption(Option6IAAddrPtr(new Option6IAAddr( + D6O_IAADDR, lease->addr_, lease->preferred_lft_, lease->valid_lft_))); - } else if (lease->type_ == Lease::TYPE_PD) { - opt->addOption(Option6IAAddrPtr(new Option6IAPrefix(D6O_IAPREFIX, + } else if (lease->type_ == Lease::TYPE_PD) { + opt->addOption(Option6IAAddrPtr(new Option6IAPrefix( + D6O_IAPREFIX, lease->addr_, lease->prefixlen_, lease->preferred_lft_, lease->valid_lft_))); - + } } } dest->addOption(opt); diff --git a/src/bin/dhcp6/tests/dhcp6_client.h b/src/bin/dhcp6/tests/dhcp6_client.h index 947ab81c5f..bec1e25d64 100644 --- a/src/bin/dhcp6/tests/dhcp6_client.h +++ b/src/bin/dhcp6/tests/dhcp6_client.h @@ -412,8 +412,12 @@ private: /// /// This method iterates over existing leases that client acquired and /// places corresponding IA_NA or IA_PD options into a specified message. - /// This is useful to construct Renew or Rebind message from the existing - /// configuration that client has obtained using 4-way exchange. + /// This is useful to construct Renew, Rebind or Confirm message from the + /// existing configuration that client has obtained using 4-way exchange. + /// + /// If there are no leases no IA options will be added. If the lease exists + /// but any of the lifetime values is set to 0, the IA option will be added + /// but the IAAddr (or IAPrefix) option will not be added. /// /// @param dest Message to which the IA options will be added. void copyIAsFromLeases(const Pkt6Ptr& dest) const; diff --git a/src/bin/dhcp6/tests/dhcp6_message_test.cc b/src/bin/dhcp6/tests/dhcp6_message_test.cc index b56c763d9c..000b65620a 100644 --- a/src/bin/dhcp6/tests/dhcp6_message_test.cc +++ b/src/bin/dhcp6/tests/dhcp6_message_test.cc @@ -70,7 +70,7 @@ Dhcpv6MessageTest::requestLease(const std::string& config, Lease6Ptr lease_server = checkLease(lease_client); ASSERT_TRUE(lease_server); // And that status code was OK. - EXPECT_EQ(STATUS_Success, client.getStatusCode(0)); + ASSERT_EQ(STATUS_Success, client.getStatusCode(0)); } } diff --git a/src/bin/dhcp6/tests/dhcp6_message_test.h b/src/bin/dhcp6/tests/dhcp6_message_test.h index e90e557529..a5a2d5e4dd 100644 --- a/src/bin/dhcp6/tests/dhcp6_message_test.h +++ b/src/bin/dhcp6/tests/dhcp6_message_test.h @@ -48,11 +48,14 @@ public: /// /// This function is called by @c bumpAddress and @c bumpSubnet. /// - /// @param input_addr An input address. + /// @warning This function is no-op if the byte index is out of range. + /// + /// @param input_addr An input address + /// @param byte_num An index of the byte which value should be increased.. /// /// @return New address. asiolink::IOAddress bumpByteInAddress(const asiolink::IOAddress& input_addr, - const size_t byte_num); + const size_t byte_num); /// @brief Increases the first byte of the address. ///