From: Tomek Mrugalski <tomasz@isc.org>
Date: Wed, 19 Apr 2017 11:23:10 +0000 (+0200)
Subject: [5087] Changes after review:
X-Git-Tag: trac5186_base~2^2~2
X-Git-Url: http://git.ipfire.org/gitweb/gitweb.cgi?a=commitdiff_plain;h=0a1a4fc3f70720ac6cba7a6bdda84030b957c978;p=thirdparty%2Fkea.git

[5087] Changes after review:
 - split FQDN test into 3 smaller ones
 - added test for truncated option
 - added sanity check for empty/truncated options
 - extended config example to showcase domain-search option
---

diff --git a/doc/examples/kea4/multiple-options.json b/doc/examples/kea4/multiple-options.json
index 7eda91fad1..b9bccb8c83 100644
--- a/doc/examples/kea4/multiple-options.json
+++ b/doc/examples/kea4/multiple-options.json
@@ -83,6 +83,14 @@
          {
              "code": 15,
              "data": "example.org"
+         },
+             // Domain search is also a popular option. It tells the client to
+             // attempt to resolve names within those specificed domains. For
+             // example, name "foo" would be attempted to be resolved as
+             // foo.mydomain.example.com and if it fails, then as foo.example.com
+         {
+             "name": "domain-search",
+             "data": "mydomain.example.com, example.com"
          },
             // String options that have a comma in their values need to have
             // it escaped (i.e. each comma is predeced by two backslashes).
diff --git a/src/lib/dhcp/option_definition.cc b/src/lib/dhcp/option_definition.cc
index 4859b80b1f..835be4f2b1 100644
--- a/src/lib/dhcp/option_definition.cc
+++ b/src/lib/dhcp/option_definition.cc
@@ -786,6 +786,9 @@ OptionDefinition::factoryFqdnList(Option::Universe u,
                                   OptionBufferConstIter end) const {
     
     const std::vector<uint8_t> data(begin, end);
+    if (data.empty()) {
+        isc_throw(InvalidOptionValue, "FQDN list option has invalid length of 0");
+    }
     InputBuffer in_buf(static_cast<const void*>(&data[0]), data.size());
     std::vector<uint8_t> out_buf;
     out_buf.reserve(data.size());
diff --git a/src/lib/dhcp/tests/libdhcp++_unittest.cc b/src/lib/dhcp/tests/libdhcp++_unittest.cc
index f22a9aefa6..a3a3010997 100644
--- a/src/lib/dhcp/tests/libdhcp++_unittest.cc
+++ b/src/lib/dhcp/tests/libdhcp++_unittest.cc
@@ -1667,7 +1667,7 @@ TEST_F(LibDhcpTest, getVendorOptionDefByName4) {
     }
 }
 
-// This test checks handling of compressed FQDN list.
+// This test checks handling of uncompressed FQDN list.
 TEST_F(LibDhcpTest, fqdnList) {
     OptionDefinitionPtr def = LibDHCP::getOptionDef(DHCP4_OPTION_SPACE,
                                                     DHO_DOMAIN_SEARCH);
@@ -1704,6 +1704,15 @@ TEST_F(LibDhcpTest, fqdnList) {
 
     LibDhcpTest::testStdOptionDefs4(DHO_DOMAIN_SEARCH, fqdn_buf.begin(),
                                     fqdn_buf.end(), typeid(OptionCustom));
+}
+
+// This test checks handling of compressed FQDN list.
+// See RFC3397, section 2 (and 4.1.4 of RFC1035 for the actual
+// compression algorithm).
+TEST_F(LibDhcpTest, fqdnListCompressed) {
+    OptionDefinitionPtr def = LibDHCP::getOptionDef(DHCP4_OPTION_SPACE,
+                                                    DHO_DOMAIN_SEARCH);
+    ASSERT_TRUE(def);
 
     const uint8_t compressed[] = {
         8, 109, 121, 100, 111, 109, 97, 105, 110, // "mydomain"
@@ -1715,19 +1724,29 @@ TEST_F(LibDhcpTest, fqdnList) {
     };
     std::vector<uint8_t> compressed_buf(compressed,
                                         compressed + sizeof(compressed));
-
+    OptionPtr option;
     ASSERT_NO_THROW(option = def->optionFactory(Option::V4,
                                                 DHO_DOMAIN_SEARCH,
                                                 compressed_buf.begin(),
                                                 compressed_buf.end()));
     ASSERT_TRUE(option);
-    names = boost::dynamic_pointer_cast<OptionCustom>(option);
+    OptionCustomPtr names = boost::dynamic_pointer_cast<OptionCustom>(option);
     ASSERT_TRUE(names);
-    EXPECT_EQ(sizeof(fqdn), names->len() - names->getHeaderLen());
+    // Why is this failing? It seems the option does not use compression.
+    EXPECT_EQ(sizeof(compressed), names->len() - names->getHeaderLen());
     ASSERT_EQ(3, names->getDataFieldsNum());
     EXPECT_EQ("mydomain.example.com.", names->readFqdn(0));
     EXPECT_EQ("example.com.", names->readFqdn(1));
     EXPECT_EQ("com.", names->readFqdn(2));
+}
+
+// Check that incorrect FQDN list compression is rejected.
+// See RFC3397, section 2 (and 4.1.4 of RFC1035 for the actual
+// compression algorithm).
+TEST_F(LibDhcpTest, fqdnListBad) {
+    OptionDefinitionPtr def = LibDHCP::getOptionDef(DHCP4_OPTION_SPACE,
+                                                    DHO_DOMAIN_SEARCH);
+    ASSERT_TRUE(def);
 
     const uint8_t bad[] = {
         8, 109, 121, 100, 111, 109, 97, 105, 110, // "mydomain"
@@ -1739,13 +1758,30 @@ TEST_F(LibDhcpTest, fqdnList) {
     };
     std::vector<uint8_t> bad_buf(bad, bad + sizeof(bad));
 
-    EXPECT_THROW(option = def->optionFactory(Option::V4, 
+    OptionPtr option;
+    EXPECT_THROW(option = def->optionFactory(Option::V4,
                                              DHO_DOMAIN_SEARCH,
                                              bad_buf.begin(),
                                              bad_buf.end()),
                  InvalidOptionValue);
 }
 
+// Check that empty (truncated) option is rejected.
+TEST_F(LibDhcpTest, fqdnListTrunc) {
+    OptionDefinitionPtr def = LibDHCP::getOptionDef(DHCP4_OPTION_SPACE,
+                                                    DHO_DOMAIN_SEARCH);
+    ASSERT_TRUE(def);
+
+    std::vector<uint8_t> empty;
+
+    OptionPtr option;
+    EXPECT_THROW(option = def->optionFactory(Option::V4,
+                                             DHO_DOMAIN_SEARCH,
+                                             empty.begin(),
+                                             empty.end()),
+                 InvalidOptionValue);
+}
+
 // tests whether v6 vendor-class option can be parsed properly.
 TEST_F(LibDhcpTest, vendorClass6) {