[3035] Skip processing invalid Hostname options sent by a client.

src/bin/dhcp4/dhcp4_messages.mes

@@ -27,6 +27,11 @@ successfully established a session with the BIND 10 control channel.
 This debug message is issued just before the IPv4 DHCP server attempts
 to establish a session with the BIND 10 control channel.
 
+% 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
+name was malformed or due to internal server error.
+
 % DHCP4_COMMAND_RECEIVED received command %1, arguments: %2
 A debug message listing the command (and possible arguments) received
 from the BIND 10 control system by the IPv4 DHCP server.
@@ -75,6 +80,11 @@ This error message is logged when the attempt to compute DHCID for a specified
 lease has failed. The lease details and reason for failure is logged in the
 message.
 
+% DHCP4_EMPTY_HOSTNAME received empty hostname from the client, skipping processing of this option
+This debug message is issued when the server received an empty Hostname option
+from a client. Server does not process empty Hostname options and therefore
+option is skipped.
+
 % DHCP4_HOOK_BUFFER_RCVD_SKIP received DHCPv4 buffer was dropped because a callout set the skip flag.
 This debug message is printed when a callout installed on buffer4_receive
 hook point set the skip flag. For this particular hook point, the

src/bin/dhcp4/dhcp4_srv.cc

@@ -33,6 +33,7 @@
 #include <dhcpsrv/utils.h>
 #include <hooks/callout_handle.h>
 #include <hooks/hooks_manager.h>
+#include <util/strutil.h>
 
 #include <boost/algorithm/string/erase.hpp>
 
@@ -747,18 +748,31 @@ Dhcpv4Srv::processClientName(const Pkt4Ptr& query, Pkt4Ptr& answer) {
     // It is possible that client has sent both Client FQDN and Hostname
     // option. In such case, server should prefer Client FQDN option and
     // ignore the Hostname option.
-    Option4ClientFqdnPtr fqdn = boost::dynamic_pointer_cast<Option4ClientFqdn>
-        (query->getOption(DHO_FQDN));
-    if (fqdn) {
-        processClientFqdnOption(fqdn, answer);
+    try {
+        Option4ClientFqdnPtr fqdn = boost::dynamic_pointer_cast<Option4ClientFqdn>
+            (query->getOption(DHO_FQDN));
+        if (fqdn) {
+            processClientFqdnOption(fqdn, answer);
 
-    } else {
-        OptionCustomPtr hostname = boost::dynamic_pointer_cast<OptionCustom>
-            (query->getOption(DHO_HOST_NAME));
-        if (hostname) {
-            processHostnameOption(hostname, answer);
-        }
+        } else {
+            OptionCustomPtr hostname = boost::dynamic_pointer_cast<OptionCustom>
+                (query->getOption(DHO_HOST_NAME));
+            if (hostname) {
+                processHostnameOption(hostname, answer);
+            }
 
+        }
+    } catch (const Exception& ex) {
+        // In some rare cases it is possible that the client's name processing
+        // fails. For example, the Hostname option may be malformed, or there
+        // may be an error in the server's logic which would cause multiple
+        // attempts to add the same option to the response message. This
+        // error message aggregates all these errors so they can be diagnosed
+        // from the log. We don't want to throw an exception here because,
+        // it will impact the processing of the whole packet. We rather want
+        // the processing to continue, even if the client's name is wrong.
+        LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL_DATA, DHCP4_CLIENT_NAME_PROC_FAIL)
+            .arg(ex.what());
     }
 }
 
@@ -855,8 +869,14 @@ Dhcpv4Srv::processHostnameOption(const OptionCustomPtr& opt_hostname,
         return;
     }
 
-    std::string hostname = opt_hostname->readString();
+    std::string hostname = isc::util::str::trim(opt_hostname->readString());
     unsigned int label_count = OptionDataTypeUtil::getLabelCount(hostname);
+    // The hostname option sent by the client should be at least 1 octet long.
+    // If it isn't we ignore this option.
+    if (label_count == 0) {
+        LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL_DATA, DHCP4_EMPTY_HOSTNAME);
+        return;
+    }
     // Copy construct the hostname provided by the client. It is entirely
     // possible that we will use the hostname option provided by the client
     // to perform the DNS update and we will send the same option to him to
@@ -870,8 +890,12 @@ Dhcpv4Srv::processHostnameOption(const OptionCustomPtr& opt_hostname,
     // By checking the number of labels present in the hostname we may infer
     // whether client has sent the fully qualified or unqualified hostname.
 
-    // If there is only one label, it is a root. We will have to generate
-    // the whole domain name for the client.
+    // @todo We may want to reconsider whether it is appropriate for the
+    // client to send a root domain name as a Hostname. There are
+    // also extensions to the auto generation of the client's name,
+    // e.g. conversion to the puny code which may be considered at some point.
+    // For now, we just remain liberal and expect that the DNS will handle
+    // conversion if needed and possible.
     if (FQDN_REPLACE_CLIENT_NAME || (label_count < 2)) {
         opt_hostname_resp->writeString("");
     // If there are two labels, it means that the client has specified

src/bin/dhcp4/tests/fqdn_unittest.cc

@@ -194,11 +194,14 @@ public:
 
     }
 
-    // Test that the Hostname option is returned in the server's response
-    // to the message holding Hostname option sent by a client.
-    void testProcessHostname(const Pkt4Ptr& query,
-                             const std::string& exp_hostname) {
-        ASSERT_TRUE(getHostnameOption(query));
+    // Processes the Hostname option in the client's message and returns
+    // the hostname option which would be sent to the client. It will
+    // throw NULL pointer if the hostname option is not to be included
+    // in the response.
+    OptionCustomPtr processHostname(const Pkt4Ptr& query) {
+        if (!getHostnameOption(query)) {
+            ADD_FAILURE() << "Hostname option not carried in the query";
+        }
 
         Pkt4Ptr answer;
         if (query->getType() == DHCPDISCOVER) {
@@ -208,16 +211,13 @@ public:
             answer.reset(new Pkt4(DHCPACK, 1234));
 
         }
-        ASSERT_NO_THROW(srv_->processClientName(query, answer));
+        srv_->processClientName(query, answer);
 
         OptionCustomPtr hostname = getHostnameOption(answer);
-        ASSERT_TRUE(hostname);
-
-        EXPECT_EQ(exp_hostname, hostname->readString());
+        return (hostname);
 
     }
 
-
     // Test that the client message holding an FQDN is processed and the
     // NameChangeRequests are generated.
     void testProcessMessageWithFqdn(const uint8_t msg_type,
@@ -356,11 +356,37 @@ TEST_F(NameDhcpv4SrvTest, serverUpdateForwardFqdn) {
 // responds with the Hostname option to confirm that the server has
 // taken responsibility for the update.
 TEST_F(NameDhcpv4SrvTest, serverUpdateHostname) {
-    Pkt4Ptr query = generatePktWithHostname(DHCPREQUEST,
-                                            "myhost.example.com.");
-    testProcessHostname(query, "myhost.example.com.");
+    Pkt4Ptr query;
+    ASSERT_NO_THROW(query = generatePktWithHostname(DHCPREQUEST,
+                                                    "myhost.example.com."));
+    OptionCustomPtr hostname;
+    ASSERT_NO_THROW(hostname = processHostname(query));
+
+    ASSERT_TRUE(hostname);
+    EXPECT_EQ("myhost.example.com.", hostname->readString());
+
+}
+
+// Test that the server skips processing of the empty Hostname option.
+TEST_F(NameDhcpv4SrvTest, serverUpdateEmptyHostname) {
+    Pkt4Ptr query;
+    ASSERT_NO_THROW(query = generatePktWithHostname(DHCPREQUEST, ""));
+    OptionCustomPtr hostname;
+    ASSERT_NO_THROW(hostname = processHostname(query));
+    EXPECT_FALSE(hostname);
+}
+
+// Test that the server skips processing of a wrong Hostname option.
+TEST_F(NameDhcpv4SrvTest, serverUpdateWrongHostname) {
+    Pkt4Ptr query;
+    ASSERT_NO_THROW(query = generatePktWithHostname(DHCPREQUEST,
+                                                    "abc..example.com"));
+    OptionCustomPtr hostname;
+    ASSERT_NO_THROW(hostname = processHostname(query));
+    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) {
@@ -380,8 +406,14 @@ TEST_F(NameDhcpv4SrvTest, serverUpdateForwardPartialNameFqdn) {
 // Test that server generates the fully qualified domain name for the client
 // if client supplies the unqualified name in the Hostname option.
 TEST_F(NameDhcpv4SrvTest, serverUpdateUnqualifiedHostname) {
-    Pkt4Ptr query = generatePktWithHostname(DHCPREQUEST, "myhost");
-    testProcessHostname(query, "myhost.example.com.");
+    Pkt4Ptr query;
+    ASSERT_NO_THROW(query = generatePktWithHostname(DHCPREQUEST, "myhost"));
+    OptionCustomPtr hostname;
+    ASSERT_NO_THROW(hostname =  processHostname(query));
+
+    ASSERT_TRUE(hostname);
+    EXPECT_EQ("myhost.example.com.", hostname->readString());
+
 }
 
 // Test that server sets empty domain-name in the FQDN option when client
@@ -587,7 +619,6 @@ TEST_F(NameDhcpv4SrvTest, processRequestEmptyHostname) {
                             0, subnet_->getValid());
 }
 
-
 // Test that client may send two requests, each carrying FQDN option with
 // a different domain-name. Server should use existing lease for the second
 // request but modify the DNS entries for the lease according to the contents

src/lib/dhcp/option_data_types.cc

@@ -229,6 +229,14 @@ OptionDataTypeUtil::writeFqdn(const std::string& fqdn,
 
 unsigned int
 OptionDataTypeUtil::getLabelCount(const std::string& text_name) {
+    // The isc::dns::Name class doesn't accept empty names. However, in some
+    // cases we may be dealing with empty names (e.g. sent by the DHCP clients).
+    // Empty names should not be sent as hostnames but if they are, for some
+    // reason, we don't want to throw an exception from this function. We
+    // rather want to signal empty name by returning 0 number of labels.
+    if (text_name.empty()) {
+        return (0);
+    }
     try {
         isc::dns::Name name(text_name);
         return (name.getLabelCount());

src/lib/dhcp/option_data_types.h

@@ -377,10 +377,13 @@ public:
 
     /// @brief Return the number of labels in the Name.
     ///
+    /// If the specified name is empty the 0 is returned.
+    ///
     /// @param text_name A text representation of the name.
     ///
-    /// @return A number of labels in the provided name.
-    /// @throw isc::BadCast if provided name is malformed.
+    /// @return A number of labels in the provided name or 0 if the
+    /// name string is empty.
+    /// @throw isc::dhcp::BadDataTypeCast if provided name is malformed.
     static unsigned int getLabelCount(const std::string& text_name);
 
     /// @brief Read string value from a buffer.

src/lib/dhcp/tests/option_data_types_unittest.cc

-// Copyright (C) 2012 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2012-2013 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -62,6 +62,20 @@ public:
     }
 };
 
+// The goal of this test is to verify that the getLabelCount returns the
+// correct number of labels in the domain name specified as a string
+// parameter.
+TEST_F(OptionDataTypesTest, getLabelCount) {
+    EXPECT_EQ(0, OptionDataTypeUtil::getLabelCount(""));
+    EXPECT_EQ(1, OptionDataTypeUtil::getLabelCount("."));
+    EXPECT_EQ(2, OptionDataTypeUtil::getLabelCount("example"));
+    EXPECT_EQ(3, OptionDataTypeUtil::getLabelCount("example.com"));
+    EXPECT_EQ(3, OptionDataTypeUtil::getLabelCount("example.com."));
+    EXPECT_EQ(4, OptionDataTypeUtil::getLabelCount("myhost.example.com"));
+    EXPECT_THROW(OptionDataTypeUtil::getLabelCount(".abc."),
+                 isc::dhcp::BadDataTypeCast);
+}
+
 // The goal of this test is to verify that an IPv4 address being
 // stored in a buffer (wire format) can be read into IOAddress
 // object.