From: Marcin Siodelski Date: Thu, 19 Mar 2015 16:40:28 +0000 (+0100) Subject: [3768] Alloc engine allocats leases to clients with repeating client ids. X-Git-Tag: kea-0.9.1~10^2~4 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=a4bc5674e946cf5924739dab16ab7946a0047338;p=thirdparty%2Fkea.git [3768] Alloc engine allocats leases to clients with repeating client ids. --- diff --git a/src/bin/dhcp4/tests/dhcp4_client.cc b/src/bin/dhcp4/tests/dhcp4_client.cc index 98d1ce20a3..dc80fb718a 100644 --- a/src/bin/dhcp4/tests/dhcp4_client.cc +++ b/src/bin/dhcp4/tests/dhcp4_client.cc @@ -262,7 +262,12 @@ Dhcp4Client::doRequest() { void Dhcp4Client::includeClientId(const std::string& clientid) { - clientid_ = ClientId::fromText(clientid); + if (clientid.empty()) { + clientid_.reset(); + + } else { + clientid_ = ClientId::fromText(clientid); + } } void @@ -410,7 +415,11 @@ Dhcp4Client::sendMsg(const Pkt4Ptr& msg) { void Dhcp4Client::setHWAddress(const std::string& hwaddr_str) { - hwaddr_.reset(new HWAddr(HWAddr::fromText(hwaddr_str))); + if (hwaddr_str.empty()) { + hwaddr_.reset(); + } else { + hwaddr_.reset(new HWAddr(HWAddr::fromText(hwaddr_str))); + } } } // end of namespace isc::dhcp::test diff --git a/src/bin/dhcp4/tests/dhcp4_client.h b/src/bin/dhcp4/tests/dhcp4_client.h index 174d1553a7..bc8ddffd0f 100644 --- a/src/bin/dhcp4/tests/dhcp4_client.h +++ b/src/bin/dhcp4/tests/dhcp4_client.h @@ -222,7 +222,8 @@ public: /// The generated client id will be added to the client's messages to the /// server. /// - /// @param clientid Client id in the textual format. + /// @param clientid Client id in the textual format. Use the empty client id + /// value to not include the client id. void includeClientId(const std::string& clientid); /// @brief Creates an instance of the Client FQDN option to be included @@ -293,7 +294,8 @@ public: /// @brief Sets the explicit hardware address for the client. /// - /// @param hwaddr_str String representation of the HW address. + /// @param hwaddr_str String representation of the HW address. Use an + /// empty string to set the NULL hardware address. void setHWAddress(const std::string& hwaddr_str); /// @brief Sets the interface over which the messages should be sent. diff --git a/src/bin/dhcp4/tests/dora_unittest.cc b/src/bin/dhcp4/tests/dora_unittest.cc index e7548a02e2..79c7314566 100644 --- a/src/bin/dhcp4/tests/dora_unittest.cc +++ b/src/bin/dhcp4/tests/dora_unittest.cc @@ -173,6 +173,16 @@ public: IfaceMgr::instance().openSockets4(); } + void oneAllocationOverlapTest(const std::string& hwaddr_a, + const std::string& clientid_a, + const std::string& hwaddr_b, + const std::string& clientid_b); + + void twoAllocationsOverlapTest(const std::string& hwaddr_a, + const std::string& clientid_a, + const std::string& hwaddr_b, + const std::string& clientid_b); + /// @brief Interface Manager's fake configuration control. IfaceMgrTestConfig iface_mgr_test_config_; @@ -441,26 +451,142 @@ TEST_F(DORATest, ciaddr) { EXPECT_EQ("0.0.0.0", resp->getCiaddr().toText()); } -TEST_F(DORATest, overlappingClientId) { - Dhcp4Client clientA(Dhcp4Client::SELECTING); - clientA.includeClientId("12:34"); - configure(DORA_CONFIGS[0], *clientA.getServer()); - ASSERT_NO_THROW(clientA.doDORA()); +void +DORATest::twoAllocationsOverlapTest(const std::string& hwaddr_a, + const std::string& clientid_a, + const std::string& hwaddr_b, + const std::string& clientid_b) { + // Allocate a lease by client A using the 4-way exchange. + Dhcp4Client client_a(Dhcp4Client::SELECTING); + client_a.includeClientId(clientid_a); + client_a.setHWAddress(hwaddr_a); + configure(DORA_CONFIGS[0], *client_a.getServer()); + ASSERT_NO_THROW(client_a.doDORA()); // Make sure that the server responded. - ASSERT_TRUE(clientA.getContext().response_); - Pkt4Ptr respA = clientA.getContext().response_; + ASSERT_TRUE(client_a.getContext().response_); + Pkt4Ptr resp_a = client_a.getContext().response_; + // Make sure that the server has responded with DHCPACK. + ASSERT_EQ(DHCPACK, static_cast(resp_a->getType())); + Lease4Ptr lease_a = LeaseMgrFactory::instance().getLease4(client_a.config_.lease_.addr_); + ASSERT_TRUE(lease_a); + + // Use the same client identifier by client B. + Dhcp4Client client_b(client_a.getServer(), Dhcp4Client::SELECTING); + client_b.setHWAddress(hwaddr_b); + client_b.includeClientId(clientid_b); + // Send DHCPDISCOVER and expect the response. + ASSERT_NO_THROW(client_b.doDiscover()); + Pkt4Ptr resp_b = client_b.getContext().response_; + // Make sure that the server has responded with DHCPOFFER. + ASSERT_EQ(DHCPOFFER, static_cast(resp_b->getType())); + // The offered address should be different than the address which + // was obtained by the client A. + ASSERT_NE(resp_b->getYiaddr(), client_a.config_.lease_.addr_); + + lease_a = LeaseMgrFactory::instance().getLease4(client_a.config_.lease_.addr_); + ASSERT_TRUE(lease_a); + + // Now that we know that the server will avoid assigning the same + // address that the client A has, use the 4-way exchange to actually + // allocate some address. + ASSERT_NO_THROW(client_b.doDORA()); + // Make sure that the server responded. + ASSERT_TRUE(client_b.getContext().response_); + resp_b = client_b.getContext().response_; // Make sure that the server has responded with DHCPACK. - ASSERT_EQ(DHCPACK, static_cast(respA->getType())); - Dhcp4Client clientB(clientA.getServer(), Dhcp4Client::SELECTING); - clientB.includeClientId("12:34"); - ASSERT_NO_THROW(clientB.doDiscover()); + ASSERT_EQ(DHCPACK, static_cast(resp_b->getType())); + // Again, make sure the assigned addresses are different. + ASSERT_NE(client_b.config_.lease_.addr_, client_a.config_.lease_.addr_); + + lease_a = LeaseMgrFactory::instance().getLease4(client_a.config_.lease_.addr_); + ASSERT_TRUE(lease_a); + Lease4Ptr lease_b = LeaseMgrFactory::instance().getLease4(client_b.config_.lease_.addr_); + ASSERT_TRUE(lease_b); + + // Client B should be able to renew its address. + client_b.setState(Dhcp4Client::RENEWING); + ASSERT_NO_THROW(client_b.doRequest()); + ASSERT_TRUE(client_b.getContext().response_); + resp_b = client_b.getContext().response_; + ASSERT_EQ(DHCPACK, static_cast(resp_b->getType())); + ASSERT_NE(client_b.config_.lease_.addr_, client_a.config_.lease_.addr_); +} - Pkt4Ptr respB = clientB.getContext().response_; - // Make sure that the server has responded with DHCPOFFER. - ASSERT_EQ(DHCPOFFER, static_cast(respB->getType())); +void +DORATest::oneAllocationOverlapTest(const std::string& hwaddr_a, + const std::string& clientid_a, + const std::string& hwaddr_b, + const std::string& clientid_b) { + // Allocate a lease by client A using the 4-way exchange. + Dhcp4Client client_a(Dhcp4Client::SELECTING); + client_a.includeClientId(clientid_a); + client_a.setHWAddress(hwaddr_a); + configure(DORA_CONFIGS[0], *client_a.getServer()); + ASSERT_NO_THROW(client_a.doDORA()); + // Make sure that the server responded. + ASSERT_TRUE(client_a.getContext().response_); + Pkt4Ptr resp_a = client_a.getContext().response_; + // Make sure that the server has responded with DHCPACK. + ASSERT_EQ(DHCPACK, static_cast(resp_a->getType())); + Lease4Ptr lease_a = LeaseMgrFactory::instance().getLease4(client_a.config_.lease_.addr_); + ASSERT_TRUE(lease_a); + + // Client B sends a DHCPDISCOVER. + Dhcp4Client client_b(client_a.getServer(), Dhcp4Client::SELECTING); + client_b.setHWAddress(hwaddr_b); + client_b.includeClientId(clientid_b); + // Send DHCPDISCOVER and expect the response. + ASSERT_NO_THROW(client_b.doDiscover()); + Pkt4Ptr resp_b = client_b.getContext().response_; + ASSERT_FALSE(resp_b); + + client_b.config_.lease_.addr_ = IOAddress::IPV4_ZERO_ADDRESS(); + client_b.setState(Dhcp4Client::INIT_REBOOT); + ASSERT_NO_THROW(client_b.doRequest()); + resp_b = client_b.getContext().response_; + ASSERT_TRUE(resp_b); + ASSERT_EQ(DHCPNAK, static_cast(resp_b->getType())); +} + +// This test checks the server behavior in the following situation: +// - Client A identifies itself to the server using client identifier +// and the hardware address and requests allocation of the new lease. +// - Server allocates the lease to the client. +// - Client B has a different hardware address but is using the same +// client identifier as Client A. +// - Client B sends DHCPDISCOVER. +// - Server should determine that the client B is not client A, because +// it is using a different hadrware address, even though they use the +// same client identifier. As a consequence, the server should offer +// a different address to the client B. +// - The client B performs the 4-way exchange again and the server +// allocates a new address to the client, which should be different +// than the address used by the client A. +// - Client B is in the renewing state and it successfully renews its +// address. +// - Client B goes to INIT_REBOOT state and asks for the address used +// by the client A. +// - The server sends DHCPNAK to refuse the allocation of this address +// to the client B. +TEST_F(DORATest, twoAllocationsOverlap1) { + twoAllocationsOverlapTest("01:02:03:04:05:06", "12:34", + "02:02:03:03:04:04", "12:34"); +} + +TEST_F(DORATest, twoAllocationsOverlap2) { + twoAllocationsOverlapTest("01:02:03:04:05:06", "12:34", + "01:02:03:04:05:06", "22:34"); +} + +TEST_F(DORATest, oneAllocationOverlap1) { + oneAllocationOverlapTest("01:02:03:04:05:06", "12:34", + "01:02:03:04:05:06", ""); +} - EXPECT_NE(clientA.config_.lease_.addr_, respB->getYiaddr()); +TEST_F(DORATest, oneAllocationOverlap2) { + oneAllocationOverlapTest("01:02:03:04:05:06", "", + "01:02:03:04:05:06", "12:34"); } // This is a simple test for the host reservation. It creates a reservation diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index e21b7840d0..f3a55217ad 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/src/lib/dhcpsrv/alloc_engine.cc @@ -1286,6 +1286,14 @@ AllocEngine::ClientContext4::myLease(const Lease4& lease) const { return (true); } +bool +AllocEngine::ClientContext4::isInConflict(const Lease4& lease) const { + return ((!(hwaddr_ && lease.hwaddr_) && (clientid_ && lease.client_id_) && + (*clientid_ == *lease.client_id_)) || + (!(clientid_ && lease.client_id_) && (hwaddr_ && lease.hwaddr_) && + (hwaddr_->hwaddr_ == lease.hwaddr_->hwaddr_))); +} + Lease4Ptr AllocEngine::allocateLease4(ClientContext4& ctx) { // The NULL pointer indicates that the old lease didn't exist. It may @@ -1347,10 +1355,20 @@ AllocEngine::discoverLease4(AllocEngine::ClientContext4& ctx) { // to either return this lease to the client or to return it as an old // (existing) lease if a different one is offered. Lease4Ptr client_lease = lease_mgr.getLease4(*ctx.hwaddr_, ctx.subnet_->getID()); - if (!client_lease && ctx.clientid_) { + if (ctx.clientid_ && ((!client_lease) || (client_lease && !ctx.myLease(*client_lease)))) { + if (client_lease && ctx.isInConflict(*client_lease)) { + return (Lease4Ptr()); + } client_lease = lease_mgr.getLease4(*ctx.clientid_, ctx.subnet_->getID()); } + if (client_lease && !ctx.myLease(*client_lease)) { + if (ctx.isInConflict(*client_lease)) { + return (Lease4Ptr()); + } + client_lease.reset(); + } + // new_lease will hold the pointer to the lease that we will offer to the // caller. Lease4Ptr new_lease; @@ -1435,13 +1453,17 @@ AllocEngine::requestLease4(AllocEngine::ClientContext4& ctx) { LeaseMgr& lease_mgr = LeaseMgrFactory::instance(); // Check if the client has any lease already. This information is needed - // to either return this lease to the client or to delete this lease if - // the new lease is allocated. + // to either return this lease to the client or to return it as an old + // (existing) lease if a different one is offered. Lease4Ptr client_lease = lease_mgr.getLease4(*ctx.hwaddr_, ctx.subnet_->getID()); - if (!client_lease && ctx.clientid_) { + if (ctx.clientid_ && ((!client_lease) || (client_lease && !ctx.myLease(*client_lease)))) { client_lease = lease_mgr.getLease4(*ctx.clientid_, ctx.subnet_->getID()); } + if (client_lease && !ctx.myLease(*client_lease)) { + client_lease.reset(); + } + // When the client sends the DHCPREQUEST, it should always specify the // address which it is requesting or renewing. That is, the client should // either use the requested IP address option or set the ciaddr. However, diff --git a/src/lib/dhcpsrv/alloc_engine.h b/src/lib/dhcpsrv/alloc_engine.h index d28c706acd..440b19bdc7 100644 --- a/src/lib/dhcpsrv/alloc_engine.h +++ b/src/lib/dhcpsrv/alloc_engine.h @@ -765,6 +765,8 @@ public: /// the context has been created, false otherwise. bool myLease(const Lease4& lease) const; + bool isInConflict(const Lease4& lease) const; + }; /// @brief Pointer to the @c ClientContext4. diff --git a/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc b/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc index 56b19104dc..99c3b1a252 100644 --- a/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc +++ b/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc @@ -542,11 +542,17 @@ TEST_F(AllocEngine4Test, requestReuseExpiredLease4) { // client (DHCPDISCOVER case). TEST_F(AllocEngine4Test, requestOtherClientLease) { // Create the first lease. - Lease4Ptr lease(new Lease4(IOAddress("192.0.2.101"), hwaddr_, 0, 0, + Lease4Ptr lease(new Lease4(IOAddress("192.0.2.101"), hwaddr_, + &clientid_->getClientId()[0], + clientid_->getClientId().size(), 100, 30, 60, time(NULL), subnet_->getID(), false, false, "")); - // Create the second lease. - Lease4Ptr lease2(new Lease4(IOAddress("192.0.2.102"), hwaddr2_, 0, 0, + // Create the second lease. Note that we use the same client id here and + // we expect that the allocation engine will figure out that the hardware + // address is different. + Lease4Ptr lease2(new Lease4(IOAddress("192.0.2.102"), hwaddr2_, + &clientid_->getClientId()[0], + clientid_->getClientId().size(), 100, 30, 60, time(NULL), subnet_->getID(), false, false, "")); // Add leases for both clients to the Lease Manager. @@ -967,7 +973,9 @@ TEST_F(AllocEngine4Test, reservedAddressExistingLeaseFakeAllocation) { CfgMgr::instance().commit(); // Create a lease for a different address than reserved. - Lease4Ptr lease(new Lease4(IOAddress("192.0.2.101"), hwaddr_, 0, 0, + Lease4Ptr lease(new Lease4(IOAddress("192.0.2.101"), hwaddr_, + &clientid_->getClientId()[0], + clientid_->getClientId().size(), 100, 30, 60, time(NULL), subnet_->getID(), false, false, "")); LeaseMgrFactory::instance().addLease(lease); @@ -1077,7 +1085,9 @@ TEST_F(AllocEngine4Test, reservedAddressExistingLeaseNoHintFakeAllocation) { CfgMgr::instance().commit(); // Create a lease for a different address than reserved. - Lease4Ptr lease(new Lease4(IOAddress("192.0.2.101"), hwaddr_, 0, 0, + Lease4Ptr lease(new Lease4(IOAddress("192.0.2.101"), hwaddr_, + &clientid_->getClientId()[0], + clientid_->getClientId().size(), 100, 30, 60, time(NULL), subnet_->getID(), false, false, "")); LeaseMgrFactory::instance().addLease(lease);