From: Marcin Siodelski <marcin@isc.org>
Date: Tue, 12 May 2015 07:54:59 +0000 (+0200)
Subject: [3747] Correctly handle client-id in the INIT-REBOOT case.
X-Git-Tag: trac3867_base~2^2~10
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=dc0d1ff68c484a1459301539b4fd71f430ae4a23;p=thirdparty%2Fkea.git

[3747] Correctly handle client-id in the INIT-REBOOT case.
---

diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc
index 073e1c33a8..e5a5183e34 100644
--- a/src/bin/dhcp4/dhcp4_srv.cc
+++ b/src/bin/dhcp4/dhcp4_srv.cc
@@ -1110,27 +1110,18 @@ Dhcpv4Srv::assignLease(Dhcpv4Exchange& ex) {
     // and whether the client is known, i.e., has a lease.
     if (!fake_allocation && !opt_serverid && opt_requested_address) {
         Lease4Ptr lease;
-        if (hwaddr) {
-            lease = LeaseMgrFactory::instance().getLease4(*hwaddr,
-                                                          subnet->getID());
+        if (client_id) {
+            lease = LeaseMgrFactory::instance().getLease4(*client_id, subnet->getID());
         }
-        if (!lease && client_id) {
-            lease = LeaseMgrFactory::instance().getLease4(*client_id,
-                                                          subnet->getID());
-        }
-        // Got a lease so we can check the address.
-        if (lease && (lease->addr_ != hint)) {
-            LOG_DEBUG(bad_packet_logger, DBG_DHCP4_DETAIL,
-                      DHCP4_PACKET_NAK_0002)
-                .arg(query->getLabel())
-                .arg(hint.toText());
 
-            resp->setType(DHCPNAK);
-            resp->setYiaddr(IOAddress::IPV4_ZERO_ADDRESS());
-            return;
+        if (!lease && hwaddr) {
+            lease = LeaseMgrFactory::instance().getLease4(*hwaddr, subnet->getID());
         }
-        // Now check the second error case: unknown client.
-        if (!lease) {
+
+        // Check the first error case: unknown client. We check this before
+        // validating the address sent because we don't want to respond if
+        // we don't know this client.
+        if (!lease || !lease->belongsToClient(hwaddr, client_id)) {
             LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL,
                       DHCP4_NO_LEASE_INIT_REBOOT)
                 .arg(hint.toText())
@@ -1140,6 +1131,19 @@ Dhcpv4Srv::assignLease(Dhcpv4Exchange& ex) {
             ex.deleteResponse();
             return;
         }
+
+        // We know this client so we can now check if his notion of the
+        // IP address is correct.
+        if (lease && (lease->addr_ != hint)) {
+            LOG_DEBUG(bad_packet_logger, DBG_DHCP4_DETAIL,
+                      DHCP4_PACKET_NAK_0002)
+                .arg(query->getLabel())
+                .arg(hint.toText());
+
+            resp->setType(DHCPNAK);
+            resp->setYiaddr(IOAddress::IPV4_ZERO_ADDRESS());
+            return;
+        }
     }
 
 
diff --git a/src/bin/dhcp4/tests/dora_unittest.cc b/src/bin/dhcp4/tests/dora_unittest.cc
index 1e9d7cb4dd..d2aa88dd3c 100644
--- a/src/bin/dhcp4/tests/dora_unittest.cc
+++ b/src/bin/dhcp4/tests/dora_unittest.cc
@@ -363,6 +363,7 @@ TEST_F(DORATest, initRebootRequest) {
     Dhcp4Client client(Dhcp4Client::SELECTING);
     // Configure DHCP server.
     configure(DORA_CONFIGS[0], *client.getServer());
+    client.includeClientId("11:22");
     // Obtain a lease from the server using the 4-way exchange.
     ASSERT_NO_THROW(client.doDORA(boost::shared_ptr<
                                   IOAddress>(new IOAddress("10.0.0.50"))));
@@ -405,11 +406,32 @@ TEST_F(DORATest, initRebootRequest) {
     resp = client.getContext().response_;
     EXPECT_EQ(DHCPNAK, static_cast<int>(resp->getType()));
 
-    // Try to request from a different client.
+    // Change client identifier. The server should treat the request
+    // as a resquest from unknown client and ignore it.
+    client.includeClientId("12:34");
+    ASSERT_NO_THROW(client.doRequest());
+    ASSERT_FALSE(client.getContext().response_);
+
+    // Now let's fix the IP address. The client identifier is still
+    // invalid so the message should be dropped.
+    client.config_.lease_.addr_ = IOAddress("10.0.0.50");
+    ASSERT_NO_THROW(client.doRequest());
+    ASSERT_FALSE(client.getContext().response_);
+
+    // Restore original client identifier.
+    client.includeClientId("11:22");
+
+    // Try to request from a different HW address. This should be successful
+    // because the client identifier matches.
     client.modifyHWAddr();
     ASSERT_NO_THROW(client.doRequest());
-    // The server should not respond.
-    EXPECT_FALSE(client.getContext().response_);
+    // Make sure that the server responded.
+    ASSERT_TRUE(client.getContext().response_);
+    resp = client.getContext().response_;
+    // Make sure that the server has responded with DHCPACK.
+    ASSERT_EQ(DHCPACK, static_cast<int>(resp->getType()));
+    // Make sure that the client has got the lease with the requested address.
+    ASSERT_EQ("10.0.0.50", client.config_.lease_.addr_.toText());
 }
 
 // Check that the ciaddr returned by the server is correct for DHCPOFFER and