[2317] More strict checks for option name format.

src/lib/dhcp/option_definition.cc

@@ -23,6 +23,8 @@
 #include <dhcp/option_int_array.h>
 #include <util/encode/hex.h>
 #include <util/strutil.h>
+#include <boost/algorithm/string/classification.hpp>
+#include <boost/algorithm/string/predicate.hpp>
 
 using namespace std;
 using namespace isc::util;
@@ -207,16 +209,29 @@ OptionDefinition::sanityCheckUniverse(const Option::Universe expected_universe,
 
 void
 OptionDefinition::validate() const {
+
+    using namespace boost::algorithm;
+
     std::ostringstream err_str;
-    if (name_.empty()) {
-        // Option name must not be empty.
-        err_str << "option name must not be empty.";
-    } else if (name_.find(" ") != string::npos) {
-        // Option name must not contain spaces.
-        err_str << "option name must not contain spaces.";
+
+    // Allowed characters in the option name are: lower or
+    // upper case letters, digits, underscores and hyphens.
+    // Empty option spaces are not allowed.
+    if (!all(name_, boost::is_from_range('a', 'z') ||
+             boost::is_from_range('A', 'Z') ||
+             boost::is_digit() ||
+             boost::is_any_of("-_")) ||
+        name_.empty() ||
+        // Hyphens and underscores are not allowed at the beginning
+        // and at the end of the option name.
+        all(find_head(name_, 1), boost::is_any_of("-_")) ||
+        all(find_tail(name_, 1), boost::is_any_of("-_"))) {
+        err_str << "invalid option name '" << name_ << "'";
+
     } else if (type_ >= OPT_UNKNOWN_TYPE) {
         // Option definition must be of a known type.
         err_str << "option type value " << type_ << " is out of range.";
+
     } else if (array_type_) {
         if (type_ == OPT_STRING_TYPE) {
             // Array of strings is not allowed because there is no way
@@ -225,9 +240,12 @@ OptionDefinition::validate() const {
             err_str << "array of strings is not a valid option definition.";
         } else if (type_ == OPT_BINARY_TYPE) {
             err_str << "array of binary values is not a valid option definition.";
+
         } else if (type_ == OPT_EMPTY_TYPE) {
             err_str << "array of empty value is not a valid option definition.";
+
         }
+
     } else if (type_ == OPT_RECORD_TYPE) {
         // At least two data fields should be added to the record. Otherwise
         // non-record option definition could be used.
@@ -235,6 +253,7 @@ OptionDefinition::validate() const {
             err_str << "invalid number of data fields: " << getRecordFields().size()
                     << " specified for the option of type 'record'. Expected at"
                     << " least 2 fields.";
+
         } else {
             // If the number of fields is valid we have to check if their order
             // is valid too. We check that string or binary data fields are not

src/lib/dhcp/tests/option_definition_unittest.cc

@@ -164,29 +164,55 @@ TEST_F(OptionDefinitionTest, validate) {
     EXPECT_THROW(opt_def5.validate(), MalformedOptionDefinition);
 
     // Option name must not contain spaces.
-    OptionDefinition opt_def6("OPTION CLIENTID", D6O_CLIENTID, "string", true);
+    OptionDefinition opt_def6("OPTION CLIENTID", D6O_CLIENTID, "string");
     EXPECT_THROW(opt_def6.validate(), MalformedOptionDefinition);
 
+    // Option name may contain lower case letters.
+    OptionDefinition opt_def7("option_clientid", D6O_CLIENTID, "string");
+    EXPECT_NO_THROW(opt_def7.validate());
+
+    // Using digits in option name is legal.
+    OptionDefinition opt_def8("option_123", D6O_CLIENTID, "string");
+    EXPECT_NO_THROW(opt_def8.validate());
+
+    // Using hyphen is legal.
+    OptionDefinition opt_def9("option-clientid", D6O_CLIENTID, "string");
+    EXPECT_NO_THROW(opt_def9.validate());
+
+    // Using hyphen or undescore at the beginning or at the end
+    // of the option name is not allowed.
+    OptionDefinition opt_def10("-option-clientid", D6O_CLIENTID, "string");
+    EXPECT_THROW(opt_def10.validate(), MalformedOptionDefinition);
+
+    OptionDefinition opt_def11("_option-clientid", D6O_CLIENTID, "string");
+    EXPECT_THROW(opt_def11.validate(), MalformedOptionDefinition);
+
+    OptionDefinition opt_def12("option-clientid_", D6O_CLIENTID, "string");
+    EXPECT_THROW(opt_def12.validate(), MalformedOptionDefinition);
+
+    OptionDefinition opt_def13("option-clientid-", D6O_CLIENTID, "string");
+    EXPECT_THROW(opt_def13.validate(), MalformedOptionDefinition);
+
     // Having array of strings does not make sense because there is no way
     // to determine string's length.
-    OptionDefinition opt_def7("OPTION_CLIENTID", D6O_CLIENTID, "string", true);
-    EXPECT_THROW(opt_def7.validate(), MalformedOptionDefinition);
+    OptionDefinition opt_def14("OPTION_CLIENTID", D6O_CLIENTID, "string", true);
+    EXPECT_THROW(opt_def14.validate(), MalformedOptionDefinition);
 
     // It does not make sense to have string field within the record before
     // other fields because there is no way to determine the length of this
     // string and thus there is no way to determine where the other field
     // begins.
-    OptionDefinition opt_def8("OPTION_STATUS_CODE", D6O_STATUS_CODE,
-                              "record");
-    opt_def8.addRecordField("string");
-    opt_def8.addRecordField("uint16");
-    EXPECT_THROW(opt_def8.validate(), MalformedOptionDefinition);
+    OptionDefinition opt_def15("OPTION_STATUS_CODE", D6O_STATUS_CODE,
+                               "record");
+    opt_def15.addRecordField("string");
+    opt_def15.addRecordField("uint16");
+    EXPECT_THROW(opt_def15.validate(), MalformedOptionDefinition);
 
     // ... but it is ok if the string value is the last one.
-    OptionDefinition opt_def9("OPTION_STATUS_CODE", D6O_STATUS_CODE,
-                              "record");
-    opt_def9.addRecordField("uint8");
-    opt_def9.addRecordField("string");
+    OptionDefinition opt_def16("OPTION_STATUS_CODE", D6O_STATUS_CODE,
+                               "record");
+    opt_def16.addRecordField("uint8");
+    opt_def16.addRecordField("string");
 }