[#4443] Extended lenient parsing of client fqdn options

Author: fxdupont

changelog_unreleased/4443-fqdn-option-flags-lenient-parsing

@@ -0,0 +1,4 @@
+[func]		fdupont
+	Extended lenient parsing of v4 "fqdn" and v6 "client-fqdn"
+	options to skip options with bad flags.
+	(Gitlab #4443)

doc/sphinx/arm/dhcp4-srv.rst

@@ -8871,7 +8871,8 @@ or in terms of the log message above, the tuple length ``y`` becomes ``x``.
     }
 
 Starting with Kea version 2.5.8, this parsing is extended to silently ignore
-FQDN (81) options with some invalid domain names.
+FQDN (81) options with some invalid domain names, and starting with Kea
+version 3.1.9 with invalid flags (i.e. 'S" and 'N' flags set to 1).
 
 Ignore DHCP Server Identifier
 -----------------------------

doc/sphinx/arm/dhcp6-srv.rst

@@ -8739,7 +8739,8 @@ MiNID.
     }
 
 Starting with Kea version 2.5.8, this parsing is extended to silently ignore
-client-fqdn (39) options with some invalid domain names.
+client-fqdn (39) options with some invalid domain names, and starting with Kea
+version	3.1.9 with invalid flags (i.e. 'S" and 'N' flags set to 1).
 
 .. _dhcp6_allocation_strategies:
 

src/lib/dhcp/option4_client_fqdn.cc

@@ -16,14 +16,14 @@
 namespace isc {
 namespace dhcp {
 
-/// @brief Implements the logic for the Option6ClientFqdn class.
+/// @brief Implements the logic for the Option4ClientFqdn class.
 ///
 /// The purpose of the class is to separate the implementation details
 /// of the Option4ClientFqdn class from the interface. This implementation
 /// uses libdns classes to process FQDNs. At some point it may be
 /// desired to split libdhcp++ from libdns. In such case the
 /// implementation of this class may be changed. The declaration of the
-/// Option6ClientFqdn class holds the pointer to implementation, so
+/// Option4ClientFqdn class holds the pointer to implementation, so
 /// the transition to a different implementation would not affect the
 /// header file.
 class Option4ClientFqdnImpl {
@@ -88,7 +88,7 @@ class Option4ClientFqdnImpl {
     /// check if the MBZ bits are set (if true). This parameter should be set
     /// to false when validating flags in the received message. This is because
     /// server should ignore MBZ bits in received messages.
-    /// @throw InvalidOption6FqdnFlags if flags are invalid.
+    /// @throw InvalidOption4FqdnFlags if flags are invalid.
     static void checkFlags(const uint8_t flags, const bool check_mbz);
 
     /// @brief Parse the Option provided in the wire format.
@@ -149,7 +149,15 @@ Option4ClientFqdnImpl::Option4ClientFqdnImpl(OptionBufferConstIter first,
     // Verify that flags value was correct. This constructor is used to parse
     // incoming packet, so don't check MBZ bits. They are ignored because we
     // don't want to discard the whole option because MBZ bits are set.
-    checkFlags(flags_, false);
+    try {
+        checkFlags(flags_, false);
+    } catch (const InvalidOption4FqdnFlags& ex) {
+        if (Option::lenient_parsing_) {
+            isc_throw(SkipThisOptionError, ex.what());
+        } else {
+            throw;
+        }
+    }
 }
 
 Option4ClientFqdnImpl::
@@ -509,7 +517,15 @@ Option4ClientFqdn::unpack(OptionBufferConstIter first,
     // Check that the flags in the received option are valid. Ignore MBZ bits,
     // because we don't want to discard the whole option because of MBZ bits
     // being set.
-    impl_->checkFlags(impl_->flags_, false);
+    try {
+        impl_->checkFlags(impl_->flags_, false);
+    } catch (const InvalidOption4FqdnFlags& ex) {
+        if (Option::lenient_parsing_) {
+            isc_throw(SkipThisOptionError, ex.what());
+        } else {
+            throw;
+        }
+    }
 }
 
 std::string

src/lib/dhcp/option6_client_fqdn.cc

@@ -122,7 +122,15 @@ Option6ClientFqdnImpl::Option6ClientFqdnImpl(OptionBufferConstIter first,
     parseWireData(first, last);
     // Verify that flags value was correct. Do not check if MBZ bits are
     // set because we should ignore those bits in received message.
-    checkFlags(flags_, false);
+    try {
+        checkFlags(flags_, false);
+    } catch (const InvalidOption6FqdnFlags& ex) {
+        if (Option::lenient_parsing_) {
+            isc_throw(SkipThisOptionError, ex.what());
+        } else {
+            throw;
+        }
+    }
 }
 
 Option6ClientFqdnImpl::
@@ -429,7 +437,15 @@ Option6ClientFqdn::unpack(OptionBufferConstIter first,
     // Check that the flags in the received option are valid. Ignore MBZ bits
     // because we don't want to discard the whole option because of MBZ bits
     // being set.
-    impl_->checkFlags(impl_->flags_, false);
+    try {
+        impl_->checkFlags(impl_->flags_, false);
+    } catch (const InvalidOption6FqdnFlags& ex) {
+        if (Option::lenient_parsing_) {
+            isc_throw(SkipThisOptionError, ex.what());
+        } else {
+            throw;
+        }
+    }
 }
 
 std::string

src/lib/dhcp/tests/option4_client_fqdn_unittest.cc

@@ -459,8 +459,12 @@ TEST(Option4ClientFqdnTest, constructFromWireInvalidFlags) {
     // Replace the flags with invalid value and verify that constructor throws
     // appropriate exception.
     in_buf[0] = Option4ClientFqdn::FLAG_N | Option4ClientFqdn::FLAG_S;
+    LenientOptionParsing lop(false);
     EXPECT_THROW(Option4ClientFqdn(in_buf.begin(), in_buf.end()),
                  InvalidOption4FqdnFlags);
+    Option::lenient_parsing_ = true;
+    EXPECT_THROW(Option4ClientFqdn(in_buf.begin(), in_buf.end()),
+                 SkipThisOptionError);
 }
 
 // This test verifies that if invalid domain name is used the constructor
@@ -834,6 +838,15 @@ TEST(Option4ClientFqdnTest, unpack) {
     EXPECT_TRUE(option->getFlag(Option4ClientFqdn::FLAG_E));
     EXPECT_EQ("myhost.example.com.", option->getDomainName());
     EXPECT_EQ(Option4ClientFqdn::FULL, option->getDomainNameType());
+
+    // Check that flags are checked.
+    in_buf[0] |= Option4ClientFqdn::FLAG_N;
+    LenientOptionParsing lop(false);
+    EXPECT_THROW(option->unpack(in_buf.begin(), in_buf.end()),
+                 InvalidOption4FqdnFlags);
+    Option::lenient_parsing_ = true;
+    EXPECT_THROW(option->unpack(in_buf.begin(), in_buf.end()),
+                 SkipThisOptionError);
 }
 
 // This test verifies that on-wire option data holding partial domain name

src/lib/dhcp/tests/option6_client_fqdn_unittest.cc

@@ -410,8 +410,12 @@ TEST(Option6ClientFqdnTest, constructFromWireInvalidFlags) {
     // Replace the flags with invalid value and verify that constructor throws
     // appropriate exception.
     in_buf[0] = Option6ClientFqdn::FLAG_N | Option6ClientFqdn::FLAG_S;
+    LenientOptionParsing lop(false);
     EXPECT_THROW(Option6ClientFqdn(in_buf.begin(), in_buf.end()),
                  InvalidOption6FqdnFlags);
+    Option::lenient_parsing_ = true;
+    EXPECT_THROW(Option6ClientFqdn(in_buf.begin(), in_buf.end()),
+                 SkipThisOptionError);
 }
 
 // This test verifies that if invalid domain name is used the constructor
@@ -702,6 +706,15 @@ TEST(Option6ClientFqdnTest, unpack) {
     EXPECT_FALSE(option->getFlag(Option6ClientFqdn::FLAG_O));
     EXPECT_EQ("myhost.example.com.", option->getDomainName());
     EXPECT_EQ(Option6ClientFqdn::FULL, option->getDomainNameType());
+
+    // Check that flags are checked.
+    in_buf[0] |= Option6ClientFqdn::FLAG_N;
+    LenientOptionParsing lop(false);
+    EXPECT_THROW(option->unpack(in_buf.begin(), in_buf.end()),
+                 InvalidOption6FqdnFlags);
+    Option::lenient_parsing_ = true;
+    EXPECT_THROW(option->unpack(in_buf.begin(), in_buf.end()),
+                 SkipThisOptionError);
 }
 
 // This test verifies that on-wire option data holding partial domain name