[66-authoritative-flag-in-kea] Cleanup code, added legacy unit test

ChangeLog

@@ -1,7 +1,8 @@
 1475.	[func]		sebschrader
-	Authoritative flag for DHCPv4 has been added. When enabled, it
+	Add authoritative feature for DHCPv4 from ISC DHCP: requests from
-	alters how DHCPv4 server treats packets from unknown clients. This
+	unknown clients are dropped (default/previous behavior) or
-	lets two servers cooperate on the same link easier.
+	answered with DHCPNAK (new behevior with new authoritative flag
+	set to true for the subnet). Patch proposed by Sebastian Schrader.
 	(Gitlab #66, git tbd)
 
 1474.	[doc]		godfryd

doc/examples/kea4/advanced.json

@@ -166,7 +166,7 @@
         },
         {
             // This particular subnet has the authoritative value changed.
-            // This casuses Kea to reply to requests with unknown IP addresses
+            // This causes Kea to reply to requests for unknown IP addresses
             // with a DHCPNAK message.
             "pools": [ { "pool": "192.0.5.100 - 192.0.5.200" } ],
             "subnet": "192.0.5.0/24",

doc/guide/dhcp4-srv.xml

@@ -3232,15 +3232,16 @@ It is merely echoed by the server
     <section xml:id="dhcp4-authoritative">
       <title>Authoritative DHCPv4 Server Behavior</title>
       <para>The original DHCPv4 specification
-      (<link xmlns:xlink="http://www.w3.org/1999/xlink" xlink:href="http://tools.ietf.org/html/rfc2131">RFC 2131</link>)
+      (<link xmlns:xlink="http://www.w3.org/1999/xlink"
+             xlink:href="http://tools.ietf.org/html/rfc2131">RFC 2131</link>)
       states that if a clients requests an address in the INIT-REBOOT state of
       which, the server has no knowledge of, the server must remain silent,
-      except if the server knows that the client requests an IP address from the
+      except if the server knows that the client requests an IP
-      wrong network.
+      address from the wrong network.
-      By default Kea follows the behavior of the ISC dhcpd instead of the
+      By default Kea follows the behavior of the ISC dhcpd instead of
-      specification and also remains silent, if the client requests an IP
+      the specification and also remains silent, if the client
-      address from the wrong network,
+      requests an IP address from the wrong network, because
-      because configuration information about a given network segment is not
+      configuration information about a given network segment is not
       known to be correct.
       Kea only rejects a client's DHCPREQUEST with a DHCPNAK message, if it
       already has a lease for the client, but with a different IP address.

src/bin/dhcp4/dhcp4_srv.cc

@@ -2004,9 +2004,9 @@ Dhcpv4Srv::assignLease(Dhcpv4Exchange& ex) {
         }
 
         // If we know this client, check if his notion of the IP address is
-        // correct, if we don't know him check, if we are authoritative.
+        // correct, if we don't know him, check if we are authoritative.
-        if ((known_client && (lease->addr_ != hint))
+        if ((known_client && (lease->addr_ != hint)) ||
-            || (!known_client && authoritative)) {
+            (!known_client && authoritative)) {
             LOG_DEBUG(bad_packet4_logger, DBG_DHCP4_DETAIL,
                       DHCP4_PACKET_NAK_0002)
                 .arg(query->getLabel())

src/bin/dhcp4/json_config_parser.cc

@@ -215,8 +215,10 @@ public:
 
                     if (authoritative != (*subnet)->getAuthoritative()) {
                         isc_throw(DhcpConfigError, "Subnet " << (*subnet)->toText()
-                                  << " has different authoritative setting " << (*subnet)->getAuthoritative()
+                                  << " has different authoritative setting "
-                                  << " than the shared-network itself: " << authoritative);
+                                  << (*subnet)->getAuthoritative()
+                                  << " than the shared-network itself: "
+                                  << authoritative);
                     }
 
                     // Let's collect the subnets in case we later find out the

src/bin/dhcp4/tests/dora_unittest.cc

@@ -138,8 +138,15 @@ namespace {
 ///   - Use for testing authoritative flag
 ///   - 1 subnet: 10.0.0.0/24
 ///   - 1 pool: 10.0.0.10-10.0.0.100
-///   - authoritative flag is set to false, thus the server responds
+///   - authoritative flag is set to true, thus the server responds
-///     with DHCPNAK to requests with unknown subnets.
+///     with DHCPNAK to requests from unknown clients.
+///
+/// - Configuration 16:
+///   - Use for testing authoritative flag
+///   - 1 subnet: 10.0.0.0/24
+///   - 1 pool: 10.0.0.10-10.0.0.100
+///   - authoritative flag is set to false, thus the server does not
+///     respond to requests from unknown clients.
 ///
 const char* DORA_CONFIGS[] = {
 // Configuration 0
@@ -506,10 +513,27 @@ const char* DORA_CONFIGS[] = {
         "      \"interfaces\": [ \"*\" ]"
         "},"
         "\"valid-lifetime\": 600,"
+        "\"authoritative\": true,"
+        "\"subnet4\": [ { "
+        "    \"subnet\": \"10.0.0.0/24\", "
+        "    \"id\": 1,"
+        "    \"pools\": [ { \"pool\": \"10.0.0.10-10.0.0.100\" } ],"
+        "    \"option-data\": [ {"
+        "        \"name\": \"routers\","
+        "        \"data\": \"10.0.0.200,10.0.0.201\""
+        "    } ]"
+        " } ]"
+    "}",
+
+// Configuration 16
+    "{ \"interfaces-config\": {"
+        "      \"interfaces\": [ \"*\" ]"
+        "},"
+        "\"valid-lifetime\": 600,"
+        "\"authoritative\": false,"
         "\"subnet4\": [ { "
         "    \"subnet\": \"10.0.0.0/24\", "
         "    \"id\": 1,"
-        "    \"authoritative\": true,"
         "    \"pools\": [ { \"pool\": \"10.0.0.10-10.0.0.100\" } ],"
         "    \"option-data\": [ {"
         "        \"name\": \"routers\","
@@ -823,7 +847,7 @@ TEST_F(DORATest, initRebootRequest) {
 
 // Test that the client in the INIT-REBOOT state can request the IP
 // address it has and the address is returned. Also, check that if
-// if the client requests invalid address the server sends a DHCPNAK.
+// if the client is unknown the server sends a DHCPNAK.
 TEST_F(DORATest, authoritative) {
     Dhcp4Client client(Dhcp4Client::SELECTING);
     // Configure DHCP server.
@@ -891,8 +915,8 @@ TEST_F(DORATest, authoritative) {
     EXPECT_EQ(DHCPNAK, static_cast<int>(resp->getType()));
 
     // Now let's fix the IP address. The client identifier is still
-    // invalid so the message should be dropped.
+    // invalid so the server still responds with DHCPNAK.
-    client.config_.lease_.addr_ = IOAddress("10.0.0.50");
+
     ASSERT_NO_THROW(client.doRequest());
     // Make sure that the server responded.
     ASSERT_TRUE(client.getContext().response_);
@@ -916,6 +940,99 @@ TEST_F(DORATest, authoritative) {
     ASSERT_EQ("10.0.0.50", client.config_.lease_.addr_.toText());
 }
 
+// Test that the client in the INIT-REBOOT state can request the IP
+// address it has and the address is returned. Also, check that if
+// if the client is unknown the request is dropped.
+TEST_F(DORATest, notAuthoritative) {
+    Dhcp4Client client(Dhcp4Client::SELECTING);
+    // Configure DHCP server.
+    configure(DORA_CONFIGS[16], *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"))));
+    // 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()));
+    // Response must not be relayed.
+    EXPECT_FALSE(resp->isRelayed());
+    // Make sure that the server id is present.
+    EXPECT_EQ("10.0.0.1", client.config_.serverid_.toText());
+    // Make sure that the client has got the lease with the requested address.
+    ASSERT_EQ("10.0.0.50", client.config_.lease_.addr_.toText());
+
+    // Client has a lease in the database. Let's transition the client
+    // to the INIT_REBOOT state so as the client can request the cached
+    // lease using the DHCPREQUEST message.
+    client.setState(Dhcp4Client::INIT_REBOOT);
+    ASSERT_NO_THROW(client.doRequest());
+
+    // 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()));
+    // Response must not be relayed.
+    EXPECT_FALSE(resp->isRelayed());
+    // Make sure that the server id is present.
+    EXPECT_EQ("10.0.0.1", client.config_.serverid_.toText());
+    // Make sure that the client has got the lease with the requested address.
+    ASSERT_EQ("10.0.0.50", client.config_.lease_.addr_.toText());
+
+    // Try to request a different address than the client has. The server
+    // should respond with DHCPNAK.
+    client.config_.lease_.addr_ = IOAddress("10.0.0.30");
+    ASSERT_NO_THROW(client.doRequest());
+    // Make sure that the server responded.
+    ASSERT_TRUE(client.getContext().response_);
+    resp = client.getContext().response_;
+    EXPECT_EQ(DHCPNAK, static_cast<int>(resp->getType()));
+
+    // Try to request another different address from an unknown subnet.
+    // The server should respond with DHCPNAK.
+    client.config_.lease_.addr_ = IOAddress("10.1.0.30");
+    ASSERT_NO_THROW(client.doRequest());
+    // Make sure that the server responded.
+    ASSERT_TRUE(client.getContext().response_);
+    resp = client.getContext().response_;
+    ASSERT_EQ(DHCPNAK, static_cast<int>(resp->getType()));
+
+    // Change client identifier. The server should treat the request
+    // as a request from unknown client and not respond (no DHCPNAK).
+    // Changed behavior vs authoritative!
+    client.includeClientId("12:34");
+    client.config_.lease_.addr_ = IOAddress("10.1.0.30");
+    ASSERT_NO_THROW(client.doRequest());
+    // Make sure that the server did not respond.
+    EXPECT_FALSE(client.getContext().response_);
+
+    // Now let's fix the IP address. The client identifier is still
+    // invalid so the message should be dropped (no DHCPNAK).
+    // Changed behavior vs authoritative!
+    client.config_.lease_.addr_ = IOAddress("10.0.0.50");
+    ASSERT_NO_THROW(client.doRequest());
+    // Make sure that the server did not respond.
+    EXPECT_FALSE(client.getContext().response_);
+
+    // Restore original client identifier.
+    client.includeClientId("11:22");
+    client.config_.lease_.addr_ = IOAddress("10.0.0.50");
+
+    // Try to request from a different HW address. This should be successful
+    // because the client identifier matches.
+    client.modifyHWAddr();
+    ASSERT_NO_THROW(client.doRequest());
+    // 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
 // DHCPNAK according to RFC2131, section 4.3.1.
 TEST_F(DORATest, ciaddr) {
@@ -2117,7 +2234,7 @@ public:
     /// Recreates PgSQL schema for a test.
     DORAPgSQLTest() : DORATest() {
         db::test::destroyPgSQLSchema();
-	db::test::createPgSQLSchema();
+        db::test::createPgSQLSchema();
     }
 
     /// @brief Destructor.

src/lib/dhcpsrv/cfgmgr.h

@@ -92,19 +92,6 @@ public:
     /// @param datadir New data directory.
     void setDataDir(const std::string& datadir);
 
-    /// @brief Sets whether server should NAK unknown clients in DHCPv4
-    ///
-    /// @param echo should unknown clients be rejected or not
-    void authoritative(const bool enabled) {
-        authoritative_ = enabled;
-    }
-
-    /// @brief Returns whether server should NAK requests for unknown leases
-    /// @return true if requests for unknown leases should be NAKed, false otherwise
-    bool authoritative() const {
-        return (authoritative_);
-    }
-
     /// @brief Updates the DHCP-DDNS client configuration to the given value.
     ///
     /// Passes the new configuration to the D2ClientMgr instance,
@@ -283,9 +270,6 @@ private:
     /// @brief directory where data files (e.g. server-id) are stored
     std::string datadir_;
 
-    /// Indicates whether v4 server should NAK requests for unknown addresses
-    bool authoritative_;
-
     /// @brief Manages the DHCP-DDNS client and its configuration.
     D2ClientMgr d2_client_mgr_;