[#4443] Extended lenient parsing of client fqdn options

diff --git a/changelog_unreleased/4443-fqdn-option-flags-lenient-parsing b/changelog_unreleased/4443-fqdn-option-flags-lenient-parsing
new file mode 100644 (file)
index 0000000..84d4876
--- /dev/null
+++ b/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)

diff --git a/doc/sphinx/arm/dhcp4-srv.rst b/doc/sphinx/arm/dhcp4-srv.rst
index 0a88bb197a2e91a6d63bfaf793af344b04be2739..bb3bf0bff54ab78507618ae13763e419ef24796e 100644 (file)
--- a/doc/sphinx/arm/dhcp4-srv.rst
+++ b/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
 -----------------------------

diff --git a/doc/sphinx/arm/dhcp6-srv.rst b/doc/sphinx/arm/dhcp6-srv.rst
index 98836532d789fada26237197352e492ebad21623..a85305ff974926ba121e0caf87cda9f6f6cb7529 100644 (file)
--- a/doc/sphinx/arm/dhcp6-srv.rst
+++ b/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:
 

diff --git a/src/lib/dhcp/option4_client_fqdn.cc b/src/lib/dhcp/option4_client_fqdn.cc
index 81edb114171dd7d5c5cb385c10eb87722a653df8..0da2503c20af791b45a5e2d3b6deaffa5ba4b087 100644 (file)
--- a/src/lib/dhcp/option4_client_fqdn.cc
+++ b/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 @@ public:
     /// 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

diff --git a/src/lib/dhcp/option6_client_fqdn.cc b/src/lib/dhcp/option6_client_fqdn.cc
index bc754f5e3f2b31da140d78bb59e082760c2d8568..2f1e71c37306ae2b6354817a7ae3810550e11d5a 100644 (file)
--- a/src/lib/dhcp/option6_client_fqdn.cc
+++ b/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

diff --git a/src/lib/dhcp/tests/option4_client_fqdn_unittest.cc b/src/lib/dhcp/tests/option4_client_fqdn_unittest.cc
index 984ffe422450f47d6fec57d4a845dbd63e2246d3..5b9ce41dbe2d57b7e344345d4a68e17103d34ea1 100644 (file)
--- a/src/lib/dhcp/tests/option4_client_fqdn_unittest.cc
+++ b/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

diff --git a/src/lib/dhcp/tests/option6_client_fqdn_unittest.cc b/src/lib/dhcp/tests/option6_client_fqdn_unittest.cc
index 20a65012200e2dca423d210c4c920ac0a41461dc..e23893d18bfcec3fd32ee63b2fccf007e5e76e49 100644 (file)
--- a/src/lib/dhcp/tests/option6_client_fqdn_unittest.cc
+++ b/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