[5440] libdhcp++ now quietly drops empty host name options from inbound packets

src/lib/dhcp/libdhcp++.cc
    LibDHCP::unpackOptions4() - added logic to drop Host Name option
    if when empty

src/lib/dhcp/tests/libdhcp++_unittest.cc
    TEST_F(LibDhcpTest, emptyHostName)  - new unit test

src/bin/dhcp4/dhcp4_srv.cc
    Dhcpv4Srv::processHostnameOption() - removed prior 5440 logic
    to ignore blank hostname options

src/bin/dhcp4/dhcp4_messages.mes
    Removed prior 5440 message

diff --git a/src/bin/dhcp4/dhcp4_messages.mes b/src/bin/dhcp4/dhcp4_messages.mes
index 83c1e21ba8d56c7e49e9b8e48d56902302062980..f0d486a6c30f11712fb343c4c092f442ae939a57 100644 (file)
--- a/src/bin/dhcp4/dhcp4_messages.mes
+++ b/src/bin/dhcp4/dhcp4_messages.mes
@@ -98,11 +98,6 @@ client and transaction identification information. The second argument
 specifies the hostname carried in the Hostname option sent by the
 client.
 
-% DHCP4_CLIENT_HOSTNAME_EMPTY %1: client sent bogus empty Hostname option
-This informational message is issued when the server receives an
-empty Hostname option. Such option is bogus (size is required to be
-at least one) and is ignored.
-
 % DHCP4_CLIENT_HOSTNAME_PROCESS %1: processing client's Hostname option
 This debug message is issued when the server starts processing the Hostname
 option sent in the client's query. The argument includes the client and

diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc
index de43e92dd700be7d36a9884dff8ca402dd787992..ab0525ae900f555df5182dd294526ac5e063952b 100644 (file)
--- a/src/bin/dhcp4/dhcp4_srv.cc
+++ b/src/bin/dhcp4/dhcp4_srv.cc
@@ -1679,13 +1679,6 @@ Dhcpv4Srv::processHostnameOption(Dhcpv4Exchange& ex) {
     OptionStringPtr opt_hostname = boost::dynamic_pointer_cast<OptionString>
         (ex.getQuery()->getOption(DHO_HOST_NAME));
 
-    // Ignore empty/bogus Hostname option.
-    if (opt_hostname && opt_hostname->getValue().empty()) {
-        opt_hostname.reset();
-        LOG_INFO(ddns4_logger, DHCP4_CLIENT_HOSTNAME_EMPTY)
-            .arg(ex.getQuery()->getLabel());
-    }
-
     if (opt_hostname) {
         LOG_DEBUG(ddns4_logger, DBG_DHCP4_DETAIL_DATA, DHCP4_CLIENT_HOSTNAME_DATA)
             .arg(ex.getQuery()->getLabel())

diff --git a/src/bin/dhcp4/tests/fqdn_unittest.cc b/src/bin/dhcp4/tests/fqdn_unittest.cc
index d66a460d4363a4e31e063c15615e10feb5a327dd..a787e4df77850300e4962a340dab9f5bba906010 100644 (file)
--- a/src/bin/dhcp4/tests/fqdn_unittest.cc
+++ b/src/bin/dhcp4/tests/fqdn_unittest.cc
@@ -836,7 +836,10 @@ TEST_F(NameDhcpv4SrvTest, serverUpdateWrongHostname) {
     EXPECT_FALSE(hostname);
 }
 
-// Test that the server skips processing of an empty Hostname option.
+// Test that the server does not see an empty Hostname option.
+// Suppressing the empty Hostname is done in libdhcp++ during
+// unpackcing, so technically we don't need this test but,
+// hey it's already written.
 TEST_F(NameDhcpv4SrvTest, serverUpdateEmptyHostname) {
     Pkt4Ptr query;
     ASSERT_NO_THROW(query = generatePktWithEmptyHostname(DHCPREQUEST));
@@ -845,7 +848,6 @@ TEST_F(NameDhcpv4SrvTest, serverUpdateEmptyHostname) {
     EXPECT_FALSE(hostname);
 }
 
-
 // Test that server generates the fully qualified domain name for the client
 // if client supplies the partial name.
 TEST_F(NameDhcpv4SrvTest, serverUpdateForwardPartialNameFqdn) {

diff --git a/src/lib/dhcp/libdhcp++.cc b/src/lib/dhcp/libdhcp++.cc
index 9ef5c11c0908107233279dfdc7248b1acd1032d1..4572270de71b77f866dca379d31b399e86c9f046 100644 (file)
--- a/src/lib/dhcp/libdhcp++.cc
+++ b/src/lib/dhcp/libdhcp++.cc
@@ -525,6 +525,15 @@ size_t LibDHCP::unpackOptions4(const OptionBuffer& buf,
             return (last_offset);
         }
 
+        // While an empty Host Name option is non-RFC compliant, some clients
+        // do send it.  In the spirit of being liberal, we'll just drop it,
+        // rather than the dropping the whole packet.  We do not have a
+        // way to log this from here but meh...  a PCAP will show it arriving,
+        // and we know we drop it.
+        if (opt_len == 0 && opt_type == DHO_HOST_NAME) {
+            continue;
+        }
+
         // Get all definitions with the particular option code. Note
         // that option code is non-unique within this container
         // however at this point we expect to get one option

diff --git a/src/lib/dhcp/tests/libdhcp++_unittest.cc b/src/lib/dhcp/tests/libdhcp++_unittest.cc
index 4dde17ba5a307c0e4ccf7782392b9b787dc97255..43f64e7bbe7915dd7934a1f3051cd9648b655ba1 100644 (file)
--- a/src/lib/dhcp/tests/libdhcp++_unittest.cc
+++ b/src/lib/dhcp/tests/libdhcp++_unittest.cc
@@ -1016,6 +1016,37 @@ TEST_F(LibDhcpTest, unpackSubOptions4) {
     EXPECT_EQ(0x0, option_bar->getValue());
 }
 
+// Verifies that an Host Name (option 12), will be dropped when empty,
+// while subsequent options will still be unpacked.
+TEST_F(LibDhcpTest, emptyHostName) {
+
+    uint8_t opts[] = {
+        12,  0,             // Empty Hostname
+        60,  3, 10, 11, 12  // Class Id
+    };
+
+    vector<uint8_t> packed(opts, opts + sizeof(opts));
+    isc::dhcp::OptionCollection options; // list of options
+    list<uint16_t> deferred;
+
+    ASSERT_NO_THROW(
+        LibDHCP::unpackOptions4(packed, "dhcp4", options, deferred);
+    );
+
+    // Host Name should not exist, we quietly drop it when empty.
+    isc::dhcp::OptionCollection::const_iterator x = options.find(12);
+    ASSERT_TRUE(x == options.end());
+
+    // Verify Option 60 exists correctly
+    x = options.find(60);
+    ASSERT_FALSE(x == options.end());
+    EXPECT_EQ(60, x->second->getType());
+    ASSERT_EQ(3, x->second->getData().size());
+    EXPECT_EQ(5, x->second->len());
+    EXPECT_EQ(0, memcmp(&x->second->getData()[0], opts + 4, 3));
+};
+
+
 TEST_F(LibDhcpTest, stdOptionDefs4) {
 
     // Create a buffer that holds dummy option data.