[3747] Use record-client-id parameter when (re)allocating leases.

src/bin/dhcp4/dhcp4_messages.mes

@@ -31,6 +31,20 @@ to establish a session with the Kea control channel.
 This debug message informs that incoming packet has been assigned to specified
 class or classes. This is a normal behavior and indicates successful operation.
 
+% DHCP4_CLIENTID_IGNORED_FOR_LEASES %1: not using client identifier for lease allocation for subnet %2
+This debug message is issued when the server is processing the DHCPv4 message
+for which client identifier will not be used when allocating new lease or
+renewing existing lease. The server is explicitly configured to not use
+client identifier to lookup existing leases for the client and will not
+record client identifier in the lease database. This mode of operation
+is useful when clients don't use stable client identifiers, e.g. multi
+stage booting. Note that the client identifier may be used for other
+operations than lease allocation, e.g. identifying host reservations
+for the client using client identifier. The first argument includes the
+client and transaction identification information. The second argument
+specifies the identifier of the subnet where the client is connected
+and for which this mode of operation is configured on the server.
+
 % DHCP4_CLIENT_NAME_PROC_FAIL failed to process the fqdn or hostname sent by a client: %1
 This debug message is issued when the DHCP server was unable to process the
 FQDN or Hostname option sent by a client. This is likely because the client's

src/bin/dhcp4/dhcp4_srv.cc

@@ -107,10 +107,22 @@ Dhcpv4Exchange::Dhcpv4Exchange(const AllocEnginePtr& alloc_engine,
     context_->subnet_ = subnet;
     // Hardware address.
     context_->hwaddr_ = query->getHWAddr();
-    // Client Identifier
-    OptionPtr opt_clientid = query->getOption(DHO_DHCP_CLIENT_IDENTIFIER);
-    if (opt_clientid) {
-        context_->clientid_.reset(new ClientId(opt_clientid->getData()));
+
+    // Set client identifier if the record-client-id flag is enabled (default).
+    // If the subnet wasn't found it doesn't matter because we will not be
+    // able to allocate a lease anyway so this context will not be used.
+    if (subnet) {
+        if (subnet->getRecordClientId()) {
+            OptionPtr opt_clientid = query->getOption(DHO_DHCP_CLIENT_IDENTIFIER);
+            if (opt_clientid) {
+                context_->clientid_.reset(new ClientId(opt_clientid->getData()));
+            }
+        } else {
+            /// @todo When merging with #3806 use different logger.
+            LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL, DHCP4_CLIENTID_IGNORED_FOR_LEASES)
+                .arg(query->getLabel())
+                .arg(subnet->getID());
+        }
     }
     // Check for static reservations.
     alloc_engine->findReservation(*context_);
@@ -1073,9 +1085,6 @@ Dhcpv4Srv::assignLease(Dhcpv4Exchange& ex) {
     LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL_DATA, DHCP4_SUBNET_SELECTED)
         .arg(subnet->toText());
 
-    // Get client-id. It is not mandatory in DHCPv4.
-    ClientIdPtr client_id = ex.getContext()->clientid_;
-
     // Get the server identifier. It will be used to determine the state
     // of the client.
     OptionCustomPtr opt_serverid = boost::dynamic_pointer_cast<
@@ -1104,6 +1113,9 @@ Dhcpv4Srv::assignLease(Dhcpv4Exchange& ex) {
     // allocation.
     bool fake_allocation = (query->getType() == DHCPDISCOVER);
 
+    // Get client-id. It is not mandatory in DHCPv4.
+    ClientIdPtr client_id = ex.getContext()->clientid_;
+
     // If there is no server id and there is a Requested IP Address option
     // the client is in the INIT-REBOOT state in which the server has to
     // determine whether the client's notion of the address is correct
@@ -1121,6 +1133,10 @@ Dhcpv4Srv::assignLease(Dhcpv4Exchange& ex) {
         // 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.
+        /// @todo The client_id logged here should be the client_id from the message
+        /// rather than effective client id which is null when the
+        /// record-client-id is set to false. This is addressed by the #3806
+        /// which is under review.
         if (!lease || !lease->belongsToClient(hwaddr, client_id)) {
             LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL,
                       DHCP4_NO_LEASE_INIT_REBOOT)
@@ -1561,7 +1577,16 @@ Dhcpv4Srv::processRelease(Pkt4Ptr& release) {
     /// @todo Uncomment this (see ticket #3116)
     /// sanityCheck(release, MANDATORY);
 
-    // Try to find client-id
+    // Try to find client-id. Note that for the DHCPRELEASE we don't check if the
+    // record-client-id configuration parameter is disabled because this parameter
+    // is configured for subnets and we don't select subnet for the DHCPRELEASE.
+    // Bogus clients usually generate new client identifiers when they first
+    // connect to the network, so whatever client identifier has been used to
+    // acquire the lease, the client identifier carried in the DHCPRELEASE is
+    // likely to be the same and the lease will be correctly identified in the
+    // lease database. If supplied client identifier differs from the one used
+    // to acquire the lease then the lease will remain in the database and
+    // simply expire.
     ClientIdPtr client_id;
     OptionPtr opt = release->getOption(DHO_DHCP_CLIENT_IDENTIFIER);
     if (opt) {

src/bin/dhcp4/tests/dora_unittest.cc

@@ -57,6 +57,13 @@ namespace {
 ///   - 1 subnet: 10.0.0.0/24
 ///   - One reservation for the client using MAC address:
 ///     aa:bb:cc:dd:ee:ff, reserved address 10.0.0.7
+///
+/// - Configuration 3:
+///   - Use for testing record-client-id flag
+///   - 1 subnet: 10.0.0.0/24
+///   - 1 pool: 10.0.0.10-10.0.0.100
+///   - record-client-id flag is set to false, thus the server
+///     uses HW address for lease lookup, rather than client id
 const char* DORA_CONFIGS[] = {
 // Configuration 0
     "{ \"interfaces-config\": {"
@@ -152,7 +159,27 @@ const char* DORA_CONFIGS[] = {
         "       }"
         "    ]"
         "} ]"
-    "}"
+    "}",
+
+// Configuration 3
+    "{ \"interfaces-config\": {"
+        "      \"interfaces\": [ \"*\" ]"
+        "},"
+        "\"valid-lifetime\": 600,"
+        "\"record-client-id\": false,"
+        "\"subnet4\": [ { "
+        "    \"subnet\": \"10.0.0.0/24\", "
+        "    \"id\": 1,"
+        "    \"pools\": [ { \"pool\": \"10.0.0.10-10.0.0.100\" } ],"
+        "    \"option-data\": [ {"
+        "        \"name\": \"routers\","
+        "        \"code\": 3,"
+        "        \"data\": \"10.0.0.200,10.0.0.201\","
+        "        \"csv-format\": true,"
+        "        \"space\": \"dhcp4\""
+        "    } ]"
+        " } ]"
+    "}",
 };
 
 /// @brief Test fixture class for testing 4-way (DORA) exchanges.
@@ -684,6 +711,78 @@ TEST_F(DORATest, reservation) {
     ASSERT_TRUE(subnet->inPool(Lease::TYPE_V4, clientB.config_.lease_.addr_));
 }
 
+// This test checks that setting the record-client-id value to false causes
+// the server to ignore changing client identifier when the client is
+// using consistent HW address.
+TEST_F(DORATest, ignoreChangingClientId) {
+    Dhcp4Client client(Dhcp4Client::SELECTING);
+    // Configure DHCP server.
+    configure(DORA_CONFIGS[3], *client.getServer());
+    client.includeClientId("12:12");
+    // Obtain the lease using 4-way exchange.
+    ASSERT_NO_THROW(client.doDORA());
+    // Make sure that the server responded.
+    ASSERT_TRUE(client.getContext().response_);
+    Pkt4Ptr resp = client.getContext().response_;
+    // Make sure that the server has responded with DHCPACK.
+    ASSERT_EQ(DHCPACK, static_cast<int>(resp->getType()));
+
+    // Remember address which the client has obtained.
+    IOAddress leased_address = client.config_.lease_.addr_;
+
+    // Modify client id. Because we have set the configuration flag which
+    // forces the server to lookup leases using the HW address, the
+    // client id modification should not matter and the client should
+    // obtain the same lease.
+    client.includeClientId("14:14");
+    // Obtain the lease using 4-way exchange.
+    ASSERT_NO_THROW(client.doDORA());
+    // 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 server assigned the same address, even though the
+    // client id has changed.
+    EXPECT_EQ(leased_address, client.config_.lease_.addr_);
+}
+
+// This test checks that the record-client-id parameter doesn't have
+// effect on the lease lookup using the HW address.
+TEST_F(DORATest, changingHWAddress) {
+    Dhcp4Client client(Dhcp4Client::SELECTING);
+    // Configure DHCP server.
+    configure(DORA_CONFIGS[3], *client.getServer());
+    client.includeClientId("12:12");
+    client.setHWAddress("00:01:02:03:04:05");
+    // Obtain the lease using 4-way exchange.
+    ASSERT_NO_THROW(client.doDORA());
+    // Make sure that the server responded.
+    ASSERT_TRUE(client.getContext().response_);
+    Pkt4Ptr resp = client.getContext().response_;
+    // Make sure that the server has responded with DHCPACK.
+    ASSERT_EQ(DHCPACK, static_cast<int>(resp->getType()));
+
+    // Remember address which the client has obtained.
+    IOAddress leased_address = client.config_.lease_.addr_;
+
+    // Modify HW address but leave client id in place. The value of the
+    // record-client-id set to false must not have any effect on the
+    // case when the HW address is changing. In such case the server will
+    // allocate the new address for the client.
+    client.setHWAddress("01:01:01:01:01:01");
+    // Obtain a lease.
+    ASSERT_NO_THROW(client.doDORA());
+    // 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()));
+    // Client must assign different address because the client id is
+    // ignored and the HW address was changed.
+    EXPECT_NE(client.config_.lease_.addr_, leased_address);
+}
+
 // This test checks the following scenario:
 // 1. Client A performs 4-way exchange and obtains a lease from the dynamic pool.
 // 2. Reservation is created for the client A using an address out of the dynamic