From: Thomas Markwalder Date: Tue, 12 Aug 2025 19:21:24 +0000 (-0400) Subject: [#4047] Remove early call to selectSubnet X-Git-Tag: Kea-3.1.1~34 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=1116c1f0f8c92d5e3047ac0a1f7185669b74fffb;p=thirdparty%2Fkea.git [#4047] Remove early call to selectSubnet /src/bin/dhcp4/dhcp4_srv.* Dhcpv4Srv::selectSubnet() Dhcpv4Srv::selectSubnet4o6() - remvoed sanity_only mode Dhcpv4Srv::assignLease() - return without NAK on non-matching rebind Dhcpv4Srv::acceptDirectRequest() - remove call to selectSubnet() /src/bin/dhcp4/tests/dhcp4_srv_unittest.cc /src/bin/dhcp4/tests/direct_client_unittest.cc /src/bin/dhcp4/tests/dora_unittest.cc /src/bin/dhcp4/tests/shared_network_unittest.cc Updated tests --- diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index 730afa3fe4..774cb9044e 100644 --- a/src/bin/dhcp4/dhcp4_srv.cc +++ b/src/bin/dhcp4/dhcp4_srv.cc @@ -773,12 +773,11 @@ Dhcpv4Srv::shutdown() { } isc::dhcp::ConstSubnet4Ptr -Dhcpv4Srv::selectSubnet(const Pkt4Ptr& query, bool& drop, - bool sanity_only, bool allow_answer_park) { +Dhcpv4Srv::selectSubnet(const Pkt4Ptr& query, bool& drop, bool allow_answer_park) { // DHCPv4-over-DHCPv6 is a special (and complex) case if (query->isDhcp4o6()) { - return (selectSubnet4o6(query, drop, sanity_only, allow_answer_park)); + return (selectSubnet4o6(query, drop, allow_answer_park)); } ConstSubnet4Ptr subnet; @@ -790,8 +789,7 @@ Dhcpv4Srv::selectSubnet(const Pkt4Ptr& query, bool& drop, // Let's execute all callouts registered for subnet4_select // (skip callouts if the selectSubnet was called to do sanity checks only) - if (!sanity_only && - HooksManager::calloutsPresent(Hooks.hook_index_subnet4_select_)) { + if (HooksManager::calloutsPresent(Hooks.hook_index_subnet4_select_)) { CalloutHandlePtr callout_handle = getCalloutHandle(query); // Use the RAII wrapper to make sure that the callout handle state is @@ -908,7 +906,7 @@ Dhcpv4Srv::selectSubnet(const Pkt4Ptr& query, bool& drop, isc::dhcp::ConstSubnet4Ptr Dhcpv4Srv::selectSubnet4o6(const Pkt4Ptr& query, bool& drop, - bool sanity_only, bool allow_answer_park) { + bool allow_answer_park) { ConstSubnet4Ptr subnet; SubnetSelector selector; @@ -958,8 +956,7 @@ Dhcpv4Srv::selectSubnet4o6(const Pkt4Ptr& query, bool& drop, // Let's execute all callouts registered for subnet4_select. // (skip callouts if the selectSubnet was called to do sanity checks only) - if (!sanity_only && - HooksManager::calloutsPresent(Hooks.hook_index_subnet4_select_)) { + if (HooksManager::calloutsPresent(Hooks.hook_index_subnet4_select_)) { CalloutHandlePtr callout_handle = getCalloutHandle(query); // Use the RAII wrapper to make sure that the callout handle state is @@ -1565,7 +1562,7 @@ Dhcpv4Srv::processDhcp4Query(Pkt4Ptr query, bool allow_answer_park) { (query->getType() == DHCPREQUEST) || (query->getType() == DHCPINFORM)) { bool drop = false; - ctx->subnet_ = selectSubnet(query, drop, false, allow_answer_park); + ctx->subnet_ = selectSubnet(query, drop, allow_answer_park); // Stop here if selectSubnet decided to drop the packet if (drop) { return (Pkt4Ptr()); @@ -3015,6 +3012,7 @@ Dhcpv4Srv::assignLease(Dhcpv4Exchange& ex) { // determine whether the client's notion of the address is correct // and whether the client is known, i.e., has a lease. auto init_reboot = (!fake_allocation && !opt_serverid && opt_requested_address); + if (init_reboot) { LOG_INFO(lease4_logger, DHCP4_INIT_REBOOT) .arg(query->getLabel()) @@ -3045,19 +3043,28 @@ Dhcpv4Srv::assignLease(Dhcpv4Exchange& ex) { // request from the INIT-REBOOT client if we're not authoritative, because // we don't know whether the network configuration is correct for this // client. We return DHCPNAK if we're authoritative, though. - if (!subnet && (!init_reboot || authoritative)) { + if (!subnet) { // This particular client is out of luck today. We do not have // information about the subnet he is connected to. This likely means // misconfiguration of the server (or some relays). - // Perhaps this should be logged on some higher level? - LOG_ERROR(bad_packet4_logger, DHCP4_PACKET_NAK_0001) - .arg(query->getLabel()) - .arg(query->getRemoteAddr().toText()) - .arg(query->getName()); - resp->setType(DHCPNAK); - resp->setYiaddr(IOAddress::IPV4_ZERO_ADDRESS()); - return; + // If it's a rebind, quietly drop it. + if (!fake_allocation && !opt_serverid && !opt_requested_address + && !query->getCiaddr().isV4Zero() && query->getLocalAddr().isV4Bcast()) { + ex.deleteResponse(); + return; + } + + if (!init_reboot || authoritative) { + // Perhaps this should be logged on some higher level? + LOG_ERROR(bad_packet4_logger, DHCP4_PACKET_NAK_0001) + .arg(query->getLabel()) + .arg(query->getRemoteAddr().toText()) + .arg(query->getName()); + resp->setType(DHCPNAK); + resp->setYiaddr(IOAddress::IPV4_ZERO_ADDRESS()); + return; + } } HWAddrPtr hwaddr = query->getHWAddr(); @@ -4640,14 +4647,8 @@ Dhcpv4Srv::acceptDirectRequest(const Pkt4Ptr& pkt) { // we validate the message type prior to calling this function. return (false); } - bool drop = false; - bool result = (!pkt->getLocalAddr().isV4Bcast() || - selectSubnet(pkt, drop, true)); - if (drop) { - // The packet must be dropped but as sanity_only is true it is dead code. - return (false); - } - return (result); + + return (true); } bool diff --git a/src/bin/dhcp4/dhcp4_srv.h b/src/bin/dhcp4/dhcp4_srv.h index 950a613f81..109ad99acc 100644 --- a/src/bin/dhcp4/dhcp4_srv.h +++ b/src/bin/dhcp4/dhcp4_srv.h @@ -1114,36 +1114,22 @@ protected: /// @brief Selects a subnet for a given client's packet. /// - /// If selectSubnet is called to simply do sanity checks (check if a - /// subnet would be selected), then there is no need to call hooks, - /// as this will happen later (when selectSubnet is called again). - /// In such case the sanity_only should be set to true. - /// /// @param query client's message /// @param drop if it is true the packet will be dropped - /// @param sanity_only if it is true the callout won't be called /// @param allow_answer_park Indicates if parking a packet is allowed /// @return selected subnet (or null if no suitable subnet was found) isc::dhcp::ConstSubnet4Ptr selectSubnet(const Pkt4Ptr& query, bool& drop, - bool sanity_only = false, bool allow_answer_park = true); /// @brief Selects a subnet for a given client's DHCP4o6 packet. /// - /// If selectSubnet is called to simply do sanity checks (check if a - /// subnet would be selected), then there is no need to call hooks, - /// as this will happen later (when selectSubnet is called again). - /// In such case the sanity_only should be set to true. - /// /// @param query client's message /// @param drop if it is true the packet will be dropped - /// @param sanity_only if it is true the callout won't be called /// @param allow_answer_park Indicates if parking a packet is allowed /// @return selected subnet (or null if no suitable subnet was found) isc::dhcp::ConstSubnet4Ptr selectSubnet4o6(const Pkt4Ptr& query, bool& drop, - bool sanity_only = false, bool allow_answer_park = true); /// @brief dummy wrapper around IfaceMgr::receive4 diff --git a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc index 549756a732..5c8caf7b64 100644 --- a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc +++ b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc @@ -4786,14 +4786,14 @@ TEST_F(Dhcpv4SrvTest, acceptDirectRequest) { pkt->setLocalAddr(IOAddress("255.255.255.255")); EXPECT_TRUE(srv_->accept(pkt)); - // For eth0, there is no subnet configured. Such message is expected - // to be silently dropped. + // If the message is broadcast it should be accepted, even though + // it has been received via eth0 for which no subnet is configured. pkt->setIface("eth0"); pkt->setIndex(ETH0_INDEX); - EXPECT_FALSE(srv_->accept(pkt)); + EXPECT_TRUE(srv_->accept(pkt)); - // But, if the message is unicast it should be accepted, even though - // it has been received via eth0. + // If the message is unicast it should be accepted, even though + // it has been received via eth0 for which no subnet is configured. pkt->setLocalAddr(IOAddress("10.0.0.1")); EXPECT_TRUE(srv_->accept(pkt)); diff --git a/src/bin/dhcp4/tests/direct_client_unittest.cc b/src/bin/dhcp4/tests/direct_client_unittest.cc index 668a5ba1c2..ff8ecf5d57 100644 --- a/src/bin/dhcp4/tests/direct_client_unittest.cc +++ b/src/bin/dhcp4/tests/direct_client_unittest.cc @@ -133,6 +133,13 @@ public: /// configured the client's message is discarded. void rebind(); + /// This test verifies that when a client in the Init-Rebooting state broadcasts + /// a Request message through an interface for which a subnet is configured, + /// the server responds to this Request. It also verifies that when such a + /// Request is sent through the interface for which there is no subnet + /// configured the client's message is discarded. + void init_reboot(); + /// @brief classes the client belongs to /// /// This is empty in most cases, but it is needed as a parameter for all @@ -369,6 +376,7 @@ DirectClientTest::renew() { // Put the client into the renewing state. client.setState(Dhcp4Client::RENEWING); + client.setDestAddress(IOAddress("10.0.0.1")); // Renew, and make sure we have obtained the same address. ASSERT_NO_THROW(client.doRequest()); @@ -413,7 +421,7 @@ DirectClientTest::rebind() { client.setIfaceName("eth1"); client.setIfaceIndex(ETH1_INDEX); ASSERT_NO_THROW(client.doRequest()); - EXPECT_FALSE(client.getContext().response_); + ASSERT_FALSE(client.getContext().response_); // Send Rebind over the correct interface, and make sure we have obtained // the same address. @@ -425,6 +433,50 @@ DirectClientTest::rebind() { EXPECT_EQ("10.0.0.10", client.config_.lease_.addr_.toText()); } +void +DirectClientTest::init_reboot() { + // Configure IfaceMgr with fake interfaces lo, eth0 and eth1. + IfaceMgrTestConfig iface_config(true); + // After creating interfaces we have to open sockets as it is required + // by the message processing code. + ASSERT_NO_THROW(IfaceMgr::instance().openSockets4()); + // Add a subnet. + ASSERT_NO_FATAL_FAILURE(configureSubnet("10.0.0.0")); + + // Create the DHCPv4 client. + Dhcp4Client client(srv_); + client.useRelay(false); + + // Obtain the lease using the 4-way exchange. + ASSERT_NO_THROW(client.doDORA(boost::shared_ptr(new IOAddress("10.0.0.10")))); + ASSERT_EQ("10.0.0.10", client.config_.lease_.addr_.toText()); + + // Put the client into the init-rebooting state. + client.setState(Dhcp4Client::INIT_REBOOT); + + // Broadcast Request through an interface for which there is no subnet + // configured. This message should be discarded by the server. + client.setIfaceName("eth1"); + client.setIfaceIndex(ETH1_INDEX); + ASSERT_NO_THROW(client.doRequest()); + ASSERT_FALSE(client.getContext().response_); + + // Send init-reboot over the correct interface, and make sure we have obtained + // the same address. + client.setIfaceName("eth0"); + client.setIfaceIndex(ETH0_INDEX); + ASSERT_NO_THROW(client.doRequest()); + ASSERT_TRUE(client.getContext().response_); + EXPECT_EQ(DHCPACK, static_cast(client.getContext().response_->getType())); + EXPECT_EQ("10.0.0.10", client.config_.lease_.addr_.toText()); + + // Send init-reboot over the wrong interface, we should get no response. + client.setIfaceName("eth1"); + client.setIfaceIndex(ETH1_INDEX); + ASSERT_NO_THROW(client.doRequest()); + ASSERT_FALSE(client.getContext().response_); +} + TEST_F(DirectClientTest, rebind) { Dhcpv4SrvMTTestGuard guard(*this, false); rebind(); @@ -435,4 +487,14 @@ TEST_F(DirectClientTest, rebindMultiThreading) { rebind(); } +TEST_F(DirectClientTest, init_reboot) { + Dhcpv4SrvMTTestGuard guard(*this, false); + init_reboot(); +} + +TEST_F(DirectClientTest, init_rebootMultiThreading) { + Dhcpv4SrvMTTestGuard guard(*this, true); + init_reboot(); +} + } diff --git a/src/bin/dhcp4/tests/dora_unittest.cc b/src/bin/dhcp4/tests/dora_unittest.cc index b2a6fdc6b3..31181492c5 100644 --- a/src/bin/dhcp4/tests/dora_unittest.cc +++ b/src/bin/dhcp4/tests/dora_unittest.cc @@ -1631,6 +1631,7 @@ DORATest::ciaddr() { ASSERT_NO_THROW(client.doRequest()); // The client is sending invalid ciaddr so the server should send a NAK. resp = client.getContext().response_; + ASSERT_TRUE(resp); ASSERT_EQ(DHCPNAK, static_cast(resp->getType())); // For DHCPNAK the ciaddr is always 0 (should not be copied) from the // client's message. diff --git a/src/bin/dhcp4/tests/shared_network_unittest.cc b/src/bin/dhcp4/tests/shared_network_unittest.cc index 47598a2b0f..053102b1ac 100644 --- a/src/bin/dhcp4/tests/shared_network_unittest.cc +++ b/src/bin/dhcp4/tests/shared_network_unittest.cc @@ -1754,7 +1754,10 @@ TEST_F(Dhcpv4SharedNetworkTest, subnetInSharedNetworkSelectedByClass) { // The client should be refused to renew the lease because it doesn't belong // to "b-devices" class. + // Set the unicast destination address to indicate that it is a renewal. client2.setState(Dhcp4Client::RENEWING); + client2.setDestAddress(IOAddress("10.0.0.1")); + testAssigned([this, &client2] { doRequest(client2, ""); });