[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 98d1ce20a398dcf534da5f178e63febf8c14f642..dc80fb718a30162497f7c8607fba33389eacc902 100644 (file)
--- 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 174d1553a74907619e41d2bf3dd57786ffa51613..bc8ddffd0f996be120e4c0390717a14255cadf1c 100644 (file)
--- 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 e7548a02e28f9da875a791e5a94b461f908ef5cc..79c73145669e4e5d2f81847ced9f28ff9afaff8e 100644 (file)
--- 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<int>(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<int>(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<int>(respA->getType()));
 
-    Dhcp4Client clientB(clientA.getServer(), Dhcp4Client::SELECTING);
-    clientB.includeClientId("12:34");
-    ASSERT_NO_THROW(clientB.doDiscover());
+    ASSERT_EQ(DHCPACK, static_cast<int>(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<int>(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<int>(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<int>(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<int>(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 e21b7840d0989e48a6d39fb420c3e4b30d7755ab..f3a55217ad21235c1a7dc1a48a876ad14682f1b7 100644 (file)
--- 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 d28c706acd432cca3f73da9c28e6c704b29557ac..440b19bdc7fb4126c3a91d36055ecb12a8316f37 100644 (file)
--- 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 56b19104dc2cdd37042a5e33ee84f19b2d851815..99c3b1a252ddcf9d6552bc39ed86b93b984c1d39 100644 (file)
--- 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);