[master] Merge branch 'trac3210' (DHCPv4 client-id echo config parameter)

Conflicts:
	ChangeLog
	doc/guide/bind10-guide.xml

ChangeLog

-7XX.	[func]		dclink, tomek
+719.	[func]		tomek
+	b10-dhcp4: Support for sending back client-id (RFC6842) has been
+	added now. Also a configuration parameter (echo-client-id) has
+	been added, so it is possible to enable backward compatibility
+	("echo-client-id false").
+	(Trac #3210, git 88a4858db206dfcd53a227562198f308f7779a72)
+
+718.	[func]		dclink, tomek
 	libdhcp++: Interface detection implemented for FreeBSD, NetBSD,
 	OpenBSD, Mac OS X and Solaris 11. Thanks to David Carlier for
 	contributing a patch.

doc/guide/bind10-guide.xml

@@ -4419,6 +4419,29 @@ Dhcp4/subnet4	[]	list	(default)
 
     </section>
 
+    <section id="dhcp4-echo-client-id">
+      <title>Echoing client-id (RFC6842)</title>
+      <para>Original DHCPv4 spec (RFC2131) states that the DHCPv4
+      server must not send back client-id options when responding to
+      clients. However, in some cases that confused clients that did
+      not have MAC address or client-id. See RFC6842 for details. That
+      behavior has changed with the publication of RFC6842 which
+      updated RFC2131. That update now states that the server must
+      send client-id if client sent it. That is the default behaviour
+      that Kea offers. However, in some cases older devices that do
+      not support RFC6842 may refuse to accept responses that include
+      client-id option. To enable backward compatibility, an optional
+      configuration parameter has been introduced. To configure it,
+      use the following commands:</para>
+
+<screen>
+&gt; <userinput>config add Dhcp4/echo-client-id</userinput>
+&gt; <userinput>config set Dhcp4/echo-client-id False</userinput>
+&gt; <userinput>config commit</userinput>
+</screen>
+
+    </section>
+
     <section id="dhcp4-std">
       <title>Supported Standards</title>
       <para>The following standards and draft standards are currently
@@ -4429,7 +4452,8 @@ Dhcp4/subnet4	[]	list	(default)
             REQUEST, RELEASE, ACK, and NAK.</simpara>
           </listitem>
           <listitem>
-            <simpara><ulink url="http://tools.ietf.org/html/rfc2132">RFC 2132</ulink>: Supported options are: PAD (0),
+            <simpara><ulink url="http://tools.ietf.org/html/rfc2132">RFC 2132</ulink>:
+            Supported options are: PAD (0),
             END(255), Message Type(53), DHCP Server Identifier (54),
             Domain Name (15), DNS Servers (6), IP Address Lease Time
             (51), Subnet mask (1), and Routers (3).</simpara>
@@ -4437,6 +4461,10 @@ Dhcp4/subnet4	[]	list	(default)
           <listitem>
             <simpara><ulink url="http://tools.ietf.org/html/rfc3046">RFC 3046</ulink>:
             Relay Agent Information option is supported.</simpara>
+            <simpara><ulink url="http://tools.ietf.org/html/rfc6842">RFC 6842</ulink>:
+            Server by default sends back client-id option. That capability may be
+            disabled. See <xref linkend="dhcp4-echo-client-id"/> for details.
+            </simpara>
           </listitem>
       </itemizedlist>
     </section>
@@ -4460,21 +4488,6 @@ Dhcp4/renew-timer	1000	integer	(default)
 &gt; <userinput>config commit</userinput></screen>
           </para>
         </listitem>
-          <listitem>
-            <simpara>During the initial IPv4 node configuration, the
-            server is expected to send packets to a node that does not
-            have IPv4 address assigned yet. The server requires
-            certain tricks (or hacks) to transmit such packets. This
-            is not implemented yet, therefore DHCPv4 server supports
-            relayed traffic only (that is, normal point to point
-            communication).</simpara>
-          </listitem>
-
-          <listitem>
-            <simpara>Upon start, the server will open sockets on all
-            interfaces that are not loopback, are up and running and
-            have IPv4 address.</simpara>
-          </listitem>
 
           <listitem>
             <simpara>The DHCPv4 server does not support

src/bin/dhcp4/config_parser.cc

@@ -216,7 +216,6 @@ protected:
         return (parser);
     }
 
-
     /// @brief Determines if the given option space name and code describe
     /// a standard option for the DCHP4 server.
     ///
@@ -395,6 +394,8 @@ DhcpConfigParser* createGlobalDhcp4ConfigParser(const std::string& config_id) {
         parser = new DbAccessParser(config_id);
     } else if (config_id.compare("hooks-libraries") == 0) {
         parser = new HooksLibrariesParser(config_id);
+    } else if (config_id.compare("echo-client-id") == 0) {
+        parser = new BooleanParser(config_id, globalContext()->boolean_values_);
     } else {
         isc_throw(NotImplemented,
                 "Parser error: Global configuration parameter not supported: "
@@ -404,6 +405,20 @@ DhcpConfigParser* createGlobalDhcp4ConfigParser(const std::string& config_id) {
     return (parser);
 }
 
+void commitGlobalOptions() {
+    // Although the function is modest for now, it is certain that the number
+    // of global switches will increase over time, hence the name.
+
+    // Set whether v4 server is supposed to echo back client-id (yes = RFC6842
+    // compatible, no = backward compatibility)
+    try {
+        bool echo_client_id = globalContext()->boolean_values_->getParam("echo-client-id");
+        CfgMgr::instance().echoClientId(echo_client_id);
+    } catch (...) {
+        // Ignore errors. This flag is optional
+    }
+}
+
 isc::data::ConstElementPtr
 configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set) {
     if (!config_set) {
@@ -536,6 +551,9 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set) {
                 iface_parser->commit();
             }
 
+            // Apply global options
+            commitGlobalOptions();
+
             // This occurs last as if it succeeds, there is no easy way
             // revert it.  As a result, the failure to commit a subsequent
             // change causes problems when trying to roll back.
@@ -579,4 +597,3 @@ ParserContextPtr& globalContext() {
 
 }; // end of isc::dhcp namespace
 }; // end of isc namespace
-

src/bin/dhcp4/dhcp4.spec

@@ -54,6 +54,12 @@
         "item_default": ""
       },
 
+      { "item_name": "echo-client-id",
+        "item_type": "boolean",
+        "item_optional": true,
+        "item_default": true
+      },
+
       { "item_name": "option-def",
         "item_type": "list",
         "item_optional": false,

src/bin/dhcp4/dhcp4_srv.cc

@@ -653,8 +653,10 @@ Dhcpv4Srv::copyDefaultFields(const Pkt4Ptr& question, Pkt4Ptr& answer) {
     answer->setGiaddr(question->getGiaddr());
 
     // Let's copy client-id to response. See RFC6842.
+    // It is possible to disable RFC6842 to keep backward compatibility
+    bool echo = CfgMgr::instance().echoClientId();
     OptionPtr client_id = question->getOption(DHO_DHCP_CLIENT_IDENTIFIER);
-    if (client_id) {
+    if (client_id && echo) {
         answer->addOption(client_id);
     }
 

src/bin/dhcp4/tests/config_parser_unittest.cc

@@ -557,6 +557,47 @@ TEST_F(Dhcp4ParserTest, nextServerOverride) {
     EXPECT_EQ("1.2.3.4", subnet->getSiaddr().toText());
 }
 
+// Check whether it is possible to configure echo-client-id
+TEST_F(Dhcp4ParserTest, echoClientId) {
+
+    ConstElementPtr status;
+
+    string config_false = "{ \"interfaces\": [ \"*\" ],"
+        "\"rebind-timer\": 2000, "
+        "\"renew-timer\": 1000, "
+        "\"echo-client-id\": false,"
+        "\"subnet4\": [ { "
+        "    \"pool\": [ \"192.0.2.1 - 192.0.2.100\" ],"
+        "    \"subnet\": \"192.0.2.0/24\" } ],"
+        "\"valid-lifetime\": 4000 }";
+
+    string config_true = "{ \"interfaces\": [ \"*\" ],"
+        "\"rebind-timer\": 2000, "
+        "\"renew-timer\": 1000, "
+        "\"echo-client-id\": true,"
+        "\"subnet4\": [ { "
+        "    \"pool\": [ \"192.0.2.1 - 192.0.2.100\" ],"
+        "    \"subnet\": \"192.0.2.0/24\" } ],"
+        "\"valid-lifetime\": 4000 }";
+
+    ElementPtr json_false = Element::fromJSON(config_false);
+    ElementPtr json_true = Element::fromJSON(config_true);
+
+    // Let's check the default. It should be true
+    ASSERT_TRUE(CfgMgr::instance().echoClientId());
+
+    // Now check that "false" configuration is really applied.
+    EXPECT_NO_THROW(status = configureDhcp4Server(*srv_, json_false));
+    ASSERT_FALSE(CfgMgr::instance().echoClientId());
+
+    // Now check that "true" configuration is really applied.
+    EXPECT_NO_THROW(status = configureDhcp4Server(*srv_, json_true));
+    ASSERT_TRUE(CfgMgr::instance().echoClientId());
+
+    // In any case revert back to the default value (true)
+    CfgMgr::instance().echoClientId(true);
+}
+
 // This test checks if it is possible to override global values
 // on a per subnet basis.
 TEST_F(Dhcp4ParserTest, subnetLocal) {

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc

@@ -608,6 +608,32 @@ TEST_F(Dhcpv4SrvTest, ManyDiscovers) {
     cout << "Offered address to client3=" << addr3.toText() << endl;
 }
 
+// Checks whether echoing back client-id is controllable, i.e.
+// whether the server obeys echo-client-id and sends (or not)
+// client-id
+TEST_F(Dhcpv4SrvTest, discoverEchoClientId) {
+    NakedDhcpv4Srv srv(0);
+
+    Pkt4Ptr dis = Pkt4Ptr(new Pkt4(DHCPDISCOVER, 1234));
+    dis->setRemoteAddr(IOAddress("192.0.2.1"));
+    OptionPtr clientid = generateClientId();
+    dis->addOption(clientid);
+
+    // Pass it to the server and get an offer
+    Pkt4Ptr offer = srv.processDiscover(dis);
+
+    // Check if we get response at all
+    checkResponse(offer, DHCPOFFER, 1234);
+    checkClientId(offer, clientid);
+
+    CfgMgr::instance().echoClientId(false);
+    offer = srv.processDiscover(dis);
+
+    // Check if we get response at all
+    checkResponse(offer, DHCPOFFER, 1234);
+    checkClientId(offer, clientid);
+}
+
 // This test verifies that incoming REQUEST can be handled properly, that an
 // ACK is generated, that the response has an address and that address
 // really belongs to the configured pool.
@@ -750,6 +776,30 @@ TEST_F(Dhcpv4SrvTest, ManyRequests) {
     cout << "Offered address to client3=" << addr3.toText() << endl;
 }
 
+// Checks whether echoing back client-id is controllable
+TEST_F(Dhcpv4SrvTest, requestEchoClientId) {
+    NakedDhcpv4Srv srv(0);
+
+    Pkt4Ptr dis = Pkt4Ptr(new Pkt4(DHCPREQUEST, 1234));
+    dis->setRemoteAddr(IOAddress("192.0.2.1"));
+    OptionPtr clientid = generateClientId();
+    dis->addOption(clientid);
+
+    // Pass it to the server and get ACK
+    Pkt4Ptr ack = srv.processRequest(dis);
+
+    // Check if we get response at all
+    checkResponse(ack, DHCPACK, 1234);
+    checkClientId(ack, clientid);
+
+    CfgMgr::instance().echoClientId(false);
+    ack = srv.processDiscover(dis);
+
+    // Check if we get response at all
+    checkResponse(ack, DHCPOFFER, 1234);
+    checkClientId(ack, clientid);
+}
+
 
 // This test verifies that incoming (positive) REQUEST/Renewing can be handled properly, that a
 // REPLY is generated, that the response has an address and that address

src/bin/dhcp4/tests/dhcp4_test_utils.cc

@@ -67,6 +67,12 @@ Dhcpv4SrvTest::Dhcpv4SrvTest()
     valid_iface_ = ifaces.begin()->getName();
 }
 
+Dhcpv4SrvTest::~Dhcpv4SrvTest() {
+
+    // Make sure that we revert to default value
+    CfgMgr::instance().echoClientId(true);
+}
+
 void Dhcpv4SrvTest::addPrlOption(Pkt4Ptr& pkt) {
 
     OptionUint8ArrayPtr option_prl =
@@ -332,12 +338,21 @@ void Dhcpv4SrvTest::checkServerId(const Pkt4Ptr& rsp, const OptionPtr& expected_
 }
 
 void Dhcpv4SrvTest::checkClientId(const Pkt4Ptr& rsp, const OptionPtr& expected_clientid) {
+
+    bool include_clientid = CfgMgr::instance().echoClientId();
+
     // check that server included our own client-id
     OptionPtr opt = rsp->getOption(DHO_DHCP_CLIENT_IDENTIFIER);
-    ASSERT_TRUE(opt);
-    EXPECT_EQ(expected_clientid->getType(), opt->getType());
-    EXPECT_EQ(expected_clientid->len(), opt->len());
-    EXPECT_TRUE(expected_clientid->getData() == opt->getData());
+    if (include_clientid) {
+        // Normal mode: echo back (see RFC6842)
+        ASSERT_TRUE(opt);
+        EXPECT_EQ(expected_clientid->getType(), opt->getType());
+        EXPECT_EQ(expected_clientid->len(), opt->len());
+        EXPECT_TRUE(expected_clientid->getData() == opt->getData());
+    } else {
+        // Backward compatibility mode for pre-RFC6842 devices
+        ASSERT_FALSE(opt);
+    }
 }
 
 ::testing::AssertionResult

src/bin/dhcp4/tests/dhcp4_test_utils.h

@@ -81,8 +81,7 @@ public:
     Dhcpv4SrvTest();
 
     /// @brief destructor
-    virtual ~Dhcpv4SrvTest() {
-    }
+    virtual ~Dhcpv4SrvTest();
 
     /// @brief Add 'Parameter Request List' option to the packet.
     ///
@@ -189,6 +188,11 @@ public:
     void checkServerId(const Pkt4Ptr& rsp, const OptionPtr& expected_srvid);
 
     /// @brief Checks if server response (OFFER, ACK, NAK) includes proper client-id
+    ///
+    /// This method follows values reported by CfgMgr in echoClientId() method.
+    /// Depending on its configuration, the client-id is either mandatory or
+    /// forbidden to appear in the response.
+    ///
     /// @param rsp response packet to be validated
     /// @param expected_clientid expected value of client-id
     void checkClientId(const Pkt4Ptr& rsp, const OptionPtr& expected_clientid);

src/lib/dhcpsrv/cfgmgr.cc

@@ -350,7 +350,7 @@ CfgMgr::getUnicast(const std::string& iface) const {
 
 CfgMgr::CfgMgr()
     : datadir_(DHCP_DATA_DIR),
-      all_ifaces_active_(false) {
+      all_ifaces_active_(false), echo_v4_client_id_(true) {
     // DHCP_DATA_DIR must be set set with -DDHCP_DATA_DIR="..." in Makefile.am
     // Note: the definition of DHCP_DATA_DIR needs to include quotation marks
     // See AM_CPPFLAGS definition in Makefile.am

src/lib/dhcpsrv/cfgmgr.h

@@ -316,6 +316,22 @@ public:
     const isc::asiolink::IOAddress*
     getUnicast(const std::string& iface) const;
 
+    /// @brief Sets whether server should send back client-id in DHCPv4
+    ///
+    /// This is a compatibility flag. The default (true) is compliant with
+    /// RFC6842. False is for backward compatibility.
+    ///
+    /// @param echo should the client-id be sent or not
+    void echoClientId(const bool echo) {
+        echo_v4_client_id_ = echo;
+    }
+
+    /// @brief Returns whether server should send back client-id in DHCPv4.
+    /// @return true if client-id should be returned, false otherwise.
+    bool echoClientId() const {
+        return (echo_v4_client_id_);
+    }
+
 protected:
 
     /// @brief Protected constructor.
@@ -392,6 +408,9 @@ private:
     /// A flag which indicates that server should listen on all available
     /// interfaces.
     bool all_ifaces_active_;
+
+    /// Indicates whether v4 server should send back client-id
+    bool echo_v4_client_id_;
 };
 
 } // namespace isc::dhcp

src/lib/dhcpsrv/tests/cfgmgr_unittest.cc

@@ -653,6 +653,23 @@ TEST_F(CfgMgrTest, activateAllIfaces) {
     EXPECT_FALSE(cfg_mgr.isActiveIface("eth2"));
 }
 
+// This test verifies that RFC6842 (echo client-id) compatibility may be
+// configured.
+TEST_F(CfgMgrTest, echoClientId) {
+    CfgMgr& cfg_mgr = CfgMgr::instance();
+
+    // Check that the default is true
+    EXPECT_TRUE(cfg_mgr.echoClientId());
+
+    // Check that it can be modified to false
+    cfg_mgr.echoClientId(false);
+    EXPECT_FALSE(cfg_mgr.echoClientId());
+
+    // Check that the default value can be restored
+    cfg_mgr.echoClientId(true);
+    EXPECT_TRUE(cfg_mgr.echoClientId());
+}
+
 /// @todo Add unit-tests for testing:
 /// - addActiveIface() with invalid interface name
 /// - addActiveIface() with the same interface twice